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

command/init: Add a new flag -lockfile=readonly #27630

Merged
merged 5 commits into from
Mar 9, 2021

Conversation

minamijoyo
Copy link
Contributor

Fixes #27506

Add a new flag -lockfile=readonly to terraform init. It would be useful to allow us to suppress dependency lockfile changes explicitly. The type of the -lockfile flag is string rather than bool, leaving room for future extensions to other behavior variants.

The readonly mode suppresses lockfile changes, but should verify checksums against the information already recorded. It should conflict with the -upgrade flag.

Note: In the original use-case described in #27506, I would like to suppress adding zh hashes, but a test code here suppresses adding h1 hashes because it's easy for testing.

@codecov
Copy link

codecov bot commented Jan 29, 2021

Codecov Report

Merging #27630 (f241d60) into main (98899df) will increase coverage by 0.03%.
The diff coverage is 85.71%.

Impacted Files Coverage Δ
internal/depsfile/locks.go 55.73% <66.66%> (+1.19%) ⬆️
command/init.go 56.39% <90.90%> (+2.56%) ⬆️
states/statefile/version4.go 58.19% <0.00%> (+0.23%) ⬆️
terraform/node_resource_plan.go 98.05% <0.00%> (+1.94%) ⬆️

@wyardley
Copy link

wyardley commented Feb 1, 2021

What about a flag to ignore it vs. not update it?

@richardj-bsquare
Copy link

An option to 'ignore' would be ideal for CI/CD systems as it somewhat avoids the multi-arch issue...

#27506 (comment)

@mumoshu
Copy link

mumoshu commented Feb 5, 2021

@wyardley @richardj-bsquare Sorry for the plug but I believe you should submit separate feature requests/pull requests your yours. Let's make separate things separate. Also, changes in this PR should be awesome foundations for your future enhancements.

I find this feature pretty solid and valid on its own 👍

@minamijoyo
Copy link
Contributor Author

@apparentlymart Could you review this?

Base automatically changed from master to main February 24, 2021 18:01
@alisdair alisdair requested a review from a team March 2, 2021 20:10
Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

Thanks for following up on this, @minamijoyo! Sorry for the delay in reviewing.

This looks good to me, with some minor notes inline from reading the code. I'd like to test it out locally and get a second opinion from someone else on @hashicorp/terraform-core.

command/init.go Outdated
@@ -767,6 +773,12 @@ func (c *InitCommand) getProviders(config *configs.Config, state *states.State,
// it's the smallest change relative to what came before it, which was
// a hidden JSON file specifically for tracking providers.)
if !newLocks.Equal(previousLocks) {
// if readonly mode, suppress changes
if flagLockfile == "readonly" {
log.Println("[DEBUG] init: detected changing dependencies, but suppressed by readonly mode")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we may want to output a warning to the UI here rather than a debug log. Maybe returning a warning diagnostic? For example:

diags = diags.Append(tfdiags.Sourceless(
  tfdiags.Warning,
  "Provider lock file not updated",
  "Changes to the provider selections were detected, but not saved in the .terraform.lock.hcl file. To record these selections, run "terraform init" without the "-lockfile=readonly" flag.,
))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -157,6 +157,7 @@ You can modify `terraform init`'s plugin behavior with the following options:
You can use `-plugin-dir` as a one-time override for exceptional situations,
such as if you are testing a local build of a provider plugin you are
currently developing.
- `-lockfile=MODE` Set dependency lockfile mode. Currently only "readonly" is valid.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to get some more detail on why you'd want to use this flag, and what the consequences are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

I tested this locally, and while the happy path worked, I found one problem. From Martin's comment on the original issue:

I think the main requirement for that option is that installation can succeed as long as all packages can be verified with information already recorded in the lock file. The option would disable Terraform from updating the file to record any new information it learned (such as a hash using a new scheme) but Terraform would still rely on and check against the information already recorded.

I don't think this requirement is met with this PR. Here's what I did:

  1. Initialized a configuration depending on only hashicorp/aws, and committed the lockfile
  2. Deleted the .terraform cache directory
  3. Added a requirement for hashicorp/random to the configuration
  4. terraform init -lockfile=readonly
  • Expected: error due to missing lockfile entry for hashicorp/random
  • Actual: initialization succeeded without error

Unless I'm misunderstanding that requirement, I think we'd need to fix this issue before merge, so I'm marking this as "request changes" for now.

Fixes hashicorp#27506

Add a new flag `-lockfile=readonly` to `terraform init`.
It would be useful to allow us to suppress dependency lockfile changes
explicitly.

The type of the `-lockfile` flag is string rather than bool, leaving
room for future extensions to other behavior variants.

The readonly mode suppresses lockfile changes, but should verify
checksums against the information already recorded. It should conflict
with the `-upgrade` flag.

Note: In the original use-case described in hashicorp#27506, I would like to
suppress adding zh hashes, but a test code here suppresses adding h1
hashes because it's easy for testing.
@minamijoyo
Copy link
Contributor Author

@alisdair Thank you for reviewing!
I added a check if required provider dependencies are changed.

Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

Thanks @minamijoyo, this looks good to me! I made a quick copy edit to one of the diagnostics to keep with our normal UI text house style, just to save another review round-trip.

@alisdair alisdair merged commit 31a5aa1 into hashicorp:main Mar 9, 2021
alisdair added a commit that referenced this pull request Mar 9, 2021
Fixes #27506

Add a new flag `-lockfile=readonly` to `terraform init`.
It would be useful to allow us to suppress dependency lockfile changes
explicitly.

The type of the `-lockfile` flag is string rather than bool, leaving
room for future extensions to other behavior variants.

The readonly mode suppresses lockfile changes, but should verify
checksums against the information already recorded. It should conflict
with the `-upgrade` flag.

Note: In the original use-case described in #27506, I would like to
suppress adding zh hashes, but a test code here suppresses adding h1
hashes because it's easy for testing.

Co-authored-by: Alisdair McDiarmid <alisdair@users.noreply.github.com>
@minamijoyo
Copy link
Contributor Author

@alisdair Thanks!

minamijoyo added a commit to minamijoyo/terraform that referenced this pull request Mar 10, 2021
Fixes hashicorp#27506

Add a new flag `-lockfile=readonly` to `terraform init`.
It would be useful to allow us to suppress dependency lockfile changes
explicitly.

The type of the `-lockfile` flag is string rather than bool, leaving
room for future extensions to other behavior variants.

The readonly mode suppresses lockfile changes, but should verify
checksums against the information already recorded. It should conflict
with the `-upgrade` flag.

Note: In the original use-case described in hashicorp#27506, I would like to
suppress adding zh hashes, but a test code here suppresses adding h1
hashes because it's easy for testing.

Co-authored-by: Alisdair McDiarmid <alisdair@users.noreply.github.com>
alisdair added a commit that referenced this pull request Mar 11, 2021
command/init: Add a new flag `-lockfile=readonly` (v0.15 backport of #27630)
@ghost
Copy link

ghost commented Apr 9, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Add a new flag terraform init -lockfile=readonly to suppress updating .terraform.lock.hcl
6 participants