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

nixos/test/ipv6: Specify port in curl #51274

Closed
wants to merge 1 commit into from

Conversation

jokogr
Copy link
Contributor

@jokogr jokogr commented Nov 30, 2018

Motivation for this change

A curl update has recently broken the IPv6 test. #50905 fixes curl and eventually the test, but the commits are on staging-next and I would like the IPv6 test to test IPv6 functionality only, so I propose to make the curl commands more defined anyway, i.e. to include the port number.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Member

@matthewbauer matthewbauer left a comment

Choose a reason for hiding this comment

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

Looks okay to me but I will let someone with more nixos knowledge merge this.

Copy link
Member

@aszlig aszlig left a comment

Choose a reason for hiding this comment

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

While testing curl is out of scope for the IPv6 test, it has however caught a real bug here (which according to this comment might the reason for the cherry-pick from upstream). So instead of adding workarounds for a thing that will cease to be a problem once #50905 lands in master I'd rather opt to either not merge this or make sure this will be reverted once ed08ff4 hits master. One way could be to add another $client->fail without the port number to make sure the test fails again once the commit lands in master.

@aszlig
Copy link
Member

aszlig commented Dec 15, 2018

Another way would be to add an assertion like:

assert !lib.any (lib.hasSuffix "fix-ipv6-url-parsing.patch") curl.patches; ...

... however that might be racy if a new upstream version of curl is released before the next staging merge.

@jokogr
Copy link
Contributor Author

jokogr commented Dec 15, 2018

@aszlig I agree that it is a good thing that the test caught a bug (albeit unexpectedly), but even nixos-unstable-small channel updates were delayed due to this failure, at least for a week if I recall correctly.

(Off-topic: is there a way to show complete history, e.g. https://hydra.nixos.org/job/nixos/unstable-small/tested#tabs-constituents show only the last builds).

I also agree with you that adding the assertion would be racy, no need to have it on this test.

If the majority still wants to test curl functionality, I will be in favor of closing this PR.

Copy link
Member

@vcunat vcunat left a comment

Choose a reason for hiding this comment

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

Yes, well I'd say I agree with aszlig, but it's certainly no strong opinion in my case.

The particular bug that broke it recently was perhaps considered not significant enough to block the small channel, but it has been unblocked in the meantime and it seems unlikely it will occur again (in a similar-enough way to be resolved by this PR).

@mmahut
Copy link
Member

mmahut commented Aug 12, 2019

Are there any updates on this pull request, please?

@domenkozar
Copy link
Member

Given that the test passes in https://hydra.nixos.org/eval/1583095?filter=ipv6&compare=1582978&full= I'm closing this

@domenkozar domenkozar closed this Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants