-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
consul: 0.9.3 -> 1.3.0 with vendored UI #49165
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.
LGTM 🐯
pkgs/servers/consul/default.nix
Outdated
|
||
buildGoPackage rec { | ||
buildGo110Package rec { |
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.
IS this a typo?
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.
if not, I don't understand where this identifier comes from :)
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.
Typo from my forward-port, fixed
Btw, my usage of
in my staging deployment suggests that the they work well with this Consul upgrade. |
And with real chunks of documentation on top of it 🎉 |
rev = "v${version}"; | ||
|
||
goPackagePath = "github.com/hashicorp/consul"; | ||
|
||
# Note: Currently only release tags are supported, because they have the Consul UI | ||
# vendored. See | ||
# https://github.com/NixOS/nixpkgs/pull/48714#issuecomment-433454834 |
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.
Perhaps add a link to #49082 here, so people know how such a build would look like?
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.
Done.
@GrahamcOfBorg build consul |
Success on aarch64-linux (full log) Attempted: consul Partial log (click to expand)
|
Success on x86_64-linux (full log) Attempted: consul Partial log (click to expand)
|
@nh2 could you rebase and solve the conflict with the release note XML please? |
I have one small change requests that we can add:
|
@arianvp Let's do that as part of a separate PR or issue, as this will require more testing. |
Removes the old UI build tooling; it is no longer necessary because as of 1.2.0 it's bundled into the server binary. It doesn't even need to have JS built, because it's bundled into the release commit's source tree (see NixOS#48714). The UI is enabled by default, so the NixOS service is updated to directly use `ui = webUi;` now. Fixes NixOS#48714. Fixes NixOS#44192. Fixes NixOS#41243. Fixes NixOS#35602. Signed-off-by: Niklas Hambüchen <[email protected]>
Signed-off-by: Niklas Hambüchen <[email protected]>
Rebased to fix release notes merge conflict |
Motivation for this change
Subsumes #48714, #44192, #41243, and #35602.
The key point is that we no longer have to build the Consul UI, it is vendored into the binary.
See commit messages for details.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)