nordvpn: init at 4.2.0 (package only)#439308
nordvpn: init at 4.2.0 (package only)#439308different-error wants to merge 2 commits intoNixOS:masterfrom
Conversation
|
@philiptaron @ruffsl fyi |
f147d3c to
27bd7db
Compare
27bd7db to
9f90e3e
Compare
|
Changes:
|
710f9ce to
2ada19d
Compare
|
Hey @different-error, I'm going to amplify @dotlambda's feedback: it's quite rude to resolve feedback threads before the feedback has been acted on. Much of the feedback in this PR has been "we can't check this in as-is; consider trying to do it another way" with different mechanisms. What it looks like from the various threads here is that you ran into issues with the suggestions then returned to what was initially proposed. I want to highlight that the most important part of the feedback was that what's currently proposed cannot be checked in. The suggestion is a helpful hint at something that might work. It's not the core of the feedback. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Thank you for your insight. To clarify, I believe your conclusion arose from the discussions in use patches and use overrideAttrs directly. With regards to the former, I didn't give up here, instead I concluded that the reviewer (who had earlier claimed a lack of knowledge of Go and therefore Go derivatives) misunderstood something. I also found it annoying that the reviewer unresolved the thread without any comment (I initially couldn't distinguish the behavior from a UI bug). With regards to the latter, I suggested using the existing change because it appeared that the OpenVPN package didn't behave as expected. I could rewrite the OpenVPN derivative locally, but that seemed overkill. To my surprise, the reviewer misinterpreted my suggestion as merging a broken PR (every commit so far in the PR builds and works as expected). Moreover, I did not conclude on my suggestion, I instead wished for guidance from someone more knowledgeable than me in this domain. I do not intend to insult anyone here and I have gratitude towards everyone's contribution. I'd like to get this package merged for the end users, some of whom have waited since 2020 for it. Time to try again. |
2ada19d to
a9b47c1
Compare
|
changes:
verified that both openvpn and nordlynx technologies work as expected. |
ruffsl
left a comment
There was a problem hiding this comment.
Retested latest changes locally. Package still building and running as expected!
a9b47c1 to
ab29c8e
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-already-reviewed/2617/2601 |
|
closing as subsumed by #477174. Thank you. |
Things done
nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Sample VM configuration.nix
This PR adds the package found in #406725 (the first two commits). Verified successful connection using openvpn and nordlynx technologies. Does not support meshnet.
Add a 👍 reaction to pull requests you find important.