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

Jump to bottom

consul-template: 0.19.4 -> 0.25.1 #108329

Merged
merged 1 commit into from Jan 5, 2021
Merged

consul-template: 0.19.4 -> 0.25.1 #108329

merged 1 commit into from Jan 5, 2021

Conversation

cpcloud
Copy link
Contributor

@cpcloud cpcloud commented Jan 3, 2021

Motivation for this change

This change updates consul-template from 0.19.4 to 0.25.1.

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
  • 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 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.

@cpcloud
Copy link
Contributor Author

cpcloud commented Jan 3, 2021

The tests are disabled here because they fail if vault is not in PATH.

@cpcloud cpcloud requested a review from ryantm January 3, 2021 16:00
@ofborg ofborg bot requested a review from kalbasit January 3, 2021 16:08
@SuperSandro2000
Copy link
Member

The tests are disabled here because they fail if vault is not in PATH.

Please add that as a comment or put vault unto checkInputs which would be preferable.

@cpcloud
Copy link
Contributor Author

cpcloud commented Jan 3, 2021

It looks like there are other reasons that tests will not work:

  1. A consul server must be running for main_test.go to run
  2. Somehow that causes an invocation of /sbin/sysctl looking for the ephemeral port range
  3. This executable does not exist in the nix sandbox

@cpcloud
Copy link
Contributor Author

cpcloud commented Jan 3, 2021 •
edited

I'll add some commentary and keep doCheck = false;

@jonringer
Copy link
Contributor

@cpcloud I think you pushed inncorrectly, you marked hte suggestions as resovled, but your force push just rebased the branch

jonringer
jonringer reviewed Jan 5, 2021
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

Please follow CONTRIBUTING.md and manual#submitting-changes-making-patches and squash the fix-up commits.

git rebase -i is a powerful command which achieves this, I created a small video demonstrating it's use here. A more indepth text tutorial can be found here

jonringer
jonringer approved these changes Jan 5, 2021
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

LGTM

Result of nixpkgs-review pr 108329 run on x86_64-linux 1

1 package built:
  • consul-template

@jonringer jonringer merged commit 014007f into NixOS:master Jan 5, 2021
@cpcloud cpcloud deleted the update-consul-template branch January 5, 2021 23:18
@cpcloud
Copy link
Contributor Author

cpcloud commented Jan 6, 2021

Thanks @jonringer!

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

@jonringer jonringer jonringer approved these changes

@ryantm ryantm Awaiting requested review from ryantm

@kalbasit kalbasit Awaiting requested review from kalbasit

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cpcloud @SuperSandro2000 @jonringer