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

terraform-providers: update and improve update script #72939

Merged
merged 4 commits into from Nov 9, 2019
Merged

terraform-providers: update and improve update script #72939

merged 4 commits into from Nov 9, 2019

Conversation

mbrgm
Copy link
Member

@mbrgm 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
Copy link
Member Author

mbrgm commented Nov 6, 2019

cc @zimbatm @stephengroat

@ofborg ofborg bot requested a review from ehmry November 6, 2019 21:34
@stephengroat
Copy link
Contributor

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
Copy link
Member

kalbasit commented Nov 8, 2019

@GrahamcOfBorg build terraform_0_12.full terraform_0_11.full

@kalbasit
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 November 8, 2019 23:40
terraform-providers: group individual repo providers
206f719
terraform-providers: fix handling of version/rev in provider list
3ddd53e 
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.
terraform-providers: update existing providers
610ffc9
terraform-provider-pass: init at 1.2.1
bc0a2ad
@mbrgm
Copy link
Member Author

mbrgm commented Nov 8, 2019

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

@mbrgm
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
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
@DzmitrySudnik DzmitrySudnik mentioned this pull request Nov 11, 2019
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehmry ehmry Awaiting requested review from ehmry

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 101-500 10.rebuild-linux: 101-500
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mbrgm @stephengroat @kalbasit @zimbatm