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

terraform-providers: update and improve update script #72939

Merged
merged 4 commits into from Nov 9, 2019

Conversation

@mbrgm
Copy link
Member

mbrgm commented Nov 6, 2019

Motivation for this change

Adding terraform-provider-pass and making it work with the update-all script required some changes to the way the update mechanism works. In the course of this I also updated all registered providers. See individual commit messages for a detailed description of reasons and measures taken.

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 nix-review --run "nix-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.
Notify maintainers

cc @

@mbrgm

This comment has been minimized.

Copy link
Member Author

mbrgm commented Nov 6, 2019

@stephengroat

This comment has been minimized.

Copy link
Contributor

stephengroat commented Nov 6, 2019

any reason not to seperate the pass provider out into a different directory, similar to ansible, etc.? i like the idea, but adding in rev seems to add a lot of duplication (over 100 lines) to cover a single case that's out of the norm for terraform-providers hosted repos, which are standard

@kalbasit

This comment has been minimized.

Copy link
Member

kalbasit commented Nov 8, 2019

@GrahamcOfBorg build terraform_0_12.full terraform_0_11.full

@kalbasit

This comment has been minimized.

Copy link
Member

kalbasit commented Nov 8, 2019

@mbrgm the commits are not following the contributing guidelines. Can you please rebase? Thanks!

Marius Bergmann added 4 commits Nov 5, 2019
I interpreted the purpose of stripping the first character from the 'version'
argument as an attempt to remove a prefixed 'v' (e.g. 'v1.0.0') from a version
tag. This works if the tag actually has a 'v' prefix, but also removes the first
character if version tags are not prefixed (e.g. '1.0.0').

Additionally, the 'v' was added again when specifying the `rev` for
`fetchFromGitHub` in default.nix. As described above, this did also not work
when provider repos did not prefix their version tags with 'v'.

I changed the implementation as follows:

- `version` and `rev` are stored inside data.nix
- `version` is used to declare the nix package version
- `rev` is used to fetch the proper git ref when building the package
- for determining `version`, an optional leading 'v' is trimmed from the tag
  name

Now this has the implication that the latest tag must always be a release tag
when using the `update-all` script, but as the result of running `update-all`
should always be reviewed before submission, makes this appear a manageable
tradeoff to me.
@mbrgm mbrgm force-pushed the mbrgm:update-terraform-providers branch from 4821b4e to bc0a2ad Nov 8, 2019
@mbrgm

This comment has been minimized.

Copy link
Member Author

mbrgm commented Nov 8, 2019

@kalbasit I'm sorry I forgot about the commit messages. Rebased.

@mbrgm

This comment has been minimized.

Copy link
Member Author

mbrgm commented Nov 8, 2019

any reason not to seperate the pass provider out into a different directory, similar to ansible, etc.? i like the idea, but adding in rev seems to add a lot of duplication (over 100 lines) to cover a single case that's out of the norm for terraform-providers hosted repos, which are standard

@stephengroat I'm sorry for not addressing this before... The method of "remove some character here, add a 'v' there" appeared a bit brittle to me in the first place. I agree with you in that terraform-providers quite nicely stick to the conventions and adding rev adds duplicate code (which is generated though, so...). On the other hand, by making update-all work with those cases as well, we can keep them inside the set of packages, which can be auto-upgraded. Extracting the pass provider to a separate directory would mean there's one additional package to upgrade manually.

I can see pros and cons in both approaches... so maybe additional opinions might help?

@zimbatm

This comment has been minimized.

Copy link
Member

zimbatm commented Nov 9, 2019

I think it's okay. Perfect is the enemy of good and all that. As long as it's maintainable.

@zimbatm zimbatm merged commit a8f50f0 into NixOS:master Nov 9, 2019
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@DzmitrySudnik DzmitrySudnik mentioned this pull request Nov 11, 2019
4 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.