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.

Sign up for GitHub

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

Jump to bottom

nixos/caddy: fix overwriting of tls settings in config #97618

Merged
merged 1 commit into from Jan 29, 2021
Merged

nixos/caddy: fix overwriting of tls settings in config #97618

merged 1 commit into from Jan 29, 2021

Conversation

lf-
Copy link
Member

@lf- lf- commented Sep 10, 2020 •
edited

This fixes a bug I reported in the thread of #97217 in which jq was used to recursively merge the nix-generated TLS settings into the config, but it was not merging arrays. I have eliminated the use of jq and instead did the config merge in Nix to maximize maintainability (since neither of us know enough jq to do it acceptably elegantly). This PR applies the jq hack as proposed in the comments.

Motivation for this change

Regression (ish) from #97217 cc @sephii

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions -- via nixops, and it's making configs which appear to have the correct settings
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests) -- my computer doesn't have kvm :(
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@sephii
Copy link
Contributor

sephii commented Sep 10, 2020

Thanks for the fix! You can see in the PR that it originally used another way of merging the JSON, which caused issues because the way the merge was done didn't create a derivation, which is the reason why jq was used in the end.

I checked your PR and the problem seems to be back, since running nix-build -A nixos/tests/caddy.nix fails like before.

@lf-
Copy link
Member Author

lf- commented Sep 10, 2020 •
edited

That's super unfortunate.

I'd appreciate if someone who knows more jq can help fix this "properly". @xfix possibly?

otherwise I threaten to rewrite this bit in Python

@lf-
Copy link
Member Author

lf- commented Sep 10, 2020 •
edited

Can confirm that the large jq command from #97217 (comment) does fix it as well.

I'm very interested in what about nix-generated JSON causes it to lose the context though, that seems like a bug? Or at least, it seems rather problematic.

@Br1ght0ne Br1ght0ne self-requested a review September 10, 2020 07:15
@worldofpeace
Copy link
Contributor

This needs a rebase.

@worldofpeace worldofpeace self-assigned this Oct 31, 2020
@lf-
Copy link
Member Author

lf- commented Oct 31, 2020

@worldofpeace done

@lf-
Copy link
Member Author

lf- commented Oct 31, 2020

So this is the "hack" version proposed in a PR comment elsewhere, as it, I think, passes the tests.

@lf- lf- requested a review from worldofpeace October 31, 2020 08:57
@worldofpeace
Copy link
Contributor

@ofborg test caddy

@worldofpeace worldofpeace merged commit 3eae73d into NixOS:master Jan 29, 2021
@worldofpeace
Copy link
Contributor

@lf- thanks for waiting and mentioning this ✨ . I think at the time the release was really eating my free time and nixos time. Good to see it's merged finally

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Br1ght0ne Br1ght0ne Awaiting requested review from Br1ght0ne

@worldofpeace worldofpeace Awaiting requested review from worldofpeace

Assignees

@worldofpeace worldofpeace

Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lf- @sephii @worldofpeace @ryantm