-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
Traefik: 1.7.14 -> 2.2.0 #76723
Traefik: 1.7.14 -> 2.2.0 #76723
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diff for package LGTM
didn't check module
executable shows usage
[5 built, 5 copied (432.5 MiB), 104.1 MiB DL]
https://github.com/NixOS/nixpkgs/pull/76723
1 package built:
traefik
Considering that the configuration isn't backwards compatible, should this maybe go into a separate package/module? traefik 1.7 is still supported upstream until having this be the only option on master and 20.03 seems reasonable Otherwise LGTM! |
Thanks for having a look on this PR. I do not understand your initial question, as in whether to have a separate package / module. As you point out after the question and I agree with this, we could maintain Traefik v1.7 on 19.09 only and v2.1 and later on 20.03 and master. Why should we have additional maintenance effort on master? @vdemeester what's your view on this? |
Yeah I'm just not sure on what the general policy is on changes like this. It's kind of two entirely different pieces of software, and 20.09 is the first release we can do where 1.7 is no longer supported upstream. I'm personally okay with 2.1 being the only thing available on master and 20.03, but I just wanted to put the info here so other's could chime in. |
Friendly pings :D (3 weeks) has this been forgotten? |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/nixos-20-03-feature-freeze/5655/17 |
''; | ||
type = types.attrs; | ||
default = { | ||
defaultEntryPoints = ["http"]; | ||
entryPoints.http.address = ":80"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you need to set entryPoints.web.address
like in the example case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I didn't see anything defaultEntryPoints
on the official documentation site, so I skipped this.
This issue on the official traefik GitHub repo supports this.
4b9b99d
to
9b7d1a1
Compare
Thanks @kalbasit for the review and sorry for the wait! I have made the following changes:
|
So, the changes look good and I tested the package update. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/130 |
Changed the module to use attrsOf instead of lazyAttrsOf as a test on 19.09
changing the hash:
EDIT: Package should build anyways, but it doesnt 20.03 should be out soon, Ill try build it ontop of that if (it doesnt get merged into it) when that happens. |
@dali99 please, do not use words like Traefik as of now requires Go 1.14 (which is the default version currently in unstable) and NixOS 19.09 merely supports 1.13 and it is not working as in unstable, as you have noticed. It happens to use this package (and NixOS module) on my servers running NixOS 19.09 and it works as intended after adding Go 1.14 in a package overlay:
The versions and the files I have copied there is left to the readers as an exercise. As I have mentioned before, I would like to focus my efforts on unstable to speed up the update process. @rnhmjoj I will try to create a NixOS test for traefik today or tomorrow. |
I'm sorry I was just trying to help 😞 Good that I was just personally failing to build it though! |
No worries, I know you meant well, but I also do not want reviewers to have an excuse for not reviewing this 😃
|
|
|
|
This commit: 1. Updates the path of the traefik package, so that the out output is used. 2. Adapts the configuration settings and options to Traefik v2. 3. Formats the NixOS traefik service using nixfmt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I don't have anything else to add: it looks pretty good to me.
I double checked the package and the moduble, everything builds and ofBorgs is happy.
Thank you for adding a test: it will make future changes easier to review.
I'll wait a bit for the other reviewers if the have anything else to say, before merging.
Motivation for this change
I was experimenting with Traefik and thought to update the package to the latest v2 one.
In the mean time,
buildGoPackage
was switched tobuildGoModule
and the respective NixOS module has been adapted to the new configuration requirements (static and dynamic, routers, middlewares, services and so on).Finally, I have removed
hamhut1066
from the maintainers list, as this user is no longer in GitHub.Fixes #76518.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)Notify maintainers
cc @vdemeester