Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

openfortivpn-webview: init at 1.2.0 #302409

Merged

Conversation

lriesebos
Copy link
Contributor

Description of changes

first contribution: added openfortivpn-webview package.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@lriesebos lriesebos force-pushed the feature/add-openfortivpn-webview branch from 91ffc8a to 43df5b6 Compare April 7, 2024 19:29
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Apr 7, 2024
Copy link
Member

@aos aos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Use the fetchFromGithub fetcher
  2. Make sure you follow commit conventions for contributing to nixpkgs: https://github.com/NixOS/nixpkgs/blob/master/pkgs/README.md#commit-conventions, specifically: rename your PR to follow the guideline above, split out the change to add yourself as the maintainer.

pkgs/by-name/op/openfortivpn-webview/package.nix Outdated Show resolved Hide resolved
@lriesebos lriesebos marked this pull request as draft April 10, 2024 04:06
Signed-off-by: Leon Riesebos <[email protected]>
@lriesebos lriesebos force-pushed the feature/add-openfortivpn-webview branch from 43df5b6 to b199dfa Compare May 24, 2024 02:48
@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label May 24, 2024
@lriesebos
Copy link
Contributor Author

@aos appreciate the feedback for a first-time contributor. sorry for the delay too, just life happened. hope this PR is in a better state now!

Copy link
Member

@aos aos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making some good progress

pkgs/by-name/op/openfortivpn-webview/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/op/openfortivpn-webview/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/op/openfortivpn-webview/package.nix Outdated Show resolved Hide resolved
@lriesebos lriesebos force-pushed the feature/add-openfortivpn-webview branch 2 times, most recently from 5a3685d to e65047d Compare June 14, 2024 01:57
@lriesebos
Copy link
Contributor Author

@aos , thanks again for the feedback, highly appreciated. let me know if there is anything else!

Copy link
Member

@aos aos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: Below is incorrect. See: NixOS/rfcs#140, but will keep below for posterity.


1. Rename from `package.nix` to `default.nix` 2. You should add the package to `pkgs/top-level/all-packages.nix`, alphabetically.

After these changes, you should be able to build it from the root of the repo with:

nix-build -A openfortivpn-webview

Once you do - test it out and make sure it works as you expect (perhaps including any version information).

I think that should be everything remaining from my point of view.

@lriesebos
Copy link
Contributor Author

@aos , I am a bit confused. the quick start to adding a package specifically asks for package.nix. also, there is no mention of all-packages.nix for adding a new package, and the new package builds fine as is. see the output below. recent PR's also seem to point in the same direction (e.g. #319908). are you sure your latest change requests are really supposed to be there?

leon@tokachi:~/code/nixpkgs$ nix-build -A openfortivpn-webview
/nix/store/9d0ssvxi5ins6as1c1x5r81pkcgp8b5l-openfortivpn-webview-1.2.0
leon@tokachi:~/code/nixpkgs$ nix build .#openfortivpn-webview
leon@tokachi:~/code/nixpkgs$ ls -l result
lrwxrwxrwx. 1 leon leon 70 Jun 15 23:08 result -> /nix/store/9d0ssvxi5ins6as1c1x5r81pkcgp8b5l-openfortivpn-webview-1.2.0
leon@tokachi:~/code/nixpkgs$ ./result/bin/openfortivpn-webview --help
openfortivpn-webview [command]

Commands:
  openfortivpn-webview [host:port]  Connect to the given host

Options:
  --version         Show version number                                [boolean]
  --realm           The authentication realm.                           [string]
  --url             The already built SAML URL. This takes precedence over
                    [host:port].                                        [string]
  --url-regex       A regex to detect the URL that needs to be visited before
                    printing SVPNCOOKIE.
                                      [string] [default: "/sslvpn/portal\.html"]
  --keep-open       Do not close the browser automatically.
  --proxy-server    HTTP Proxy in the format hostname:port.             [string]
  --extra-ca-certs  Path to a file with extra certificates. The file should
                    consist of one or more trusted certificates in PEM format.
                                                                        [string]
  --trusted-cert    The fingerprint of a certificate to always trust, even if
                    invalid. The details of invalid certificates, fingerprint
                    included, will be dumped in the console.            [string]
  --help            Show help                                          [boolean]

Copy link
Member

@aos aos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lriesebos Ah thanks for the correction. Looks like RFC 140 was merged somewhat recently and the guidelines have also been updated. In that case - disregard my last comment. This is good to go from my point of view :-)

@lriesebos lriesebos marked this pull request as ready for review June 16, 2024 03:47
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jun 20, 2024
@lriesebos
Copy link
Contributor Author

if there is anything else that needs to be done to unblock this, let me know

@aos
Copy link
Member

aos commented Jul 24, 2024

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1865

@SuperSandro2000 SuperSandro2000 changed the title pkgs/tools/networking/openfortivpn-webview: added openfortivpn-webview 1.2.0 openfortivpn-webview: added openfortivpn-webview 1.2.0 Jul 28, 2024
@SuperSandro2000 SuperSandro2000 changed the title openfortivpn-webview: added openfortivpn-webview 1.2.0 openfortivpn-webview: init at 1.2.0 Jul 28, 2024
@lriesebos lriesebos force-pushed the feature/add-openfortivpn-webview branch from e65047d to 27171a9 Compare July 29, 2024 02:20
@lriesebos lriesebos force-pushed the feature/add-openfortivpn-webview branch from 27171a9 to 24436c5 Compare July 29, 2024 02:31
Release version v1.2.0-electron

Added the new `--trusted-cert` option to bypass certificate errors.

Signed-off-by: Leon Riesebos <[email protected]>
@lriesebos lriesebos force-pushed the feature/add-openfortivpn-webview branch from 24436c5 to a45a079 Compare July 29, 2024 02:35
@lriesebos
Copy link
Contributor Author

@SuperSandro2000 I think I addressed your last concerns. let me know if there is anything else I can do!

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 7, 2024
@SuperSandro2000
Copy link
Member

@ofborg build openfortivpn-webview

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Dec 3, 2024
@FliegendeWurst FliegendeWurst merged commit d8d3c38 into NixOS:master Dec 23, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 2 This PR was reviewed and approved by two reputable people 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants