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

Fix regression - Delete .terraform.lock.hcl if it was created by terraform init #1701

Merged
merged 2 commits into from Aug 30, 2021

Conversation

gezb
Copy link
Contributor

@gezb gezb commented Jul 13, 2021

This PR fixes a regression I introduced with the changes in #1651 reported in #1698

My initial change did not take into account that the terraform init ran as part of the workflow will create the .terraform.lock.hcl lock file and the knock on affects of this file existing when the project is re-planned

If the lock file exists after terraform init has run and it did not exist initially we delete it

@gezb gezb requested a review from a team as a code owner July 13, 2021 21:58
@gezb gezb force-pushed the feature/fix_re-plan_regression branch from 475b59b to 2aa183d Compare July 13, 2021 22:39
@gezb gezb changed the title Delete .terraform.lock.hcl if it was created by terraform init Fix regression - Delete .terraform.lock.hcl if it was created by terraform init Jul 13, 2021
outputs, err := p.runSteps(ctx.Steps, ctx, projAbsPath)
if err != nil {
if unlockErr := lockAttempt.UnlockFn(); unlockErr != nil {
ctx.Log.Err("error unlocking state after plan error: %v", unlockErr)
}
if !lockFileExistsProirToPlan && common.FileExists(lockfilePath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if !lockFileExistsProirToPlan && common.FileExists(lockfilePath) {
if !lockFileExistsPriorToPlan && common.FileExists(lockfilePath) {

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

outputs, err := p.runSteps(ctx.Steps, ctx, projAbsPath)
if err != nil {
if unlockErr := lockAttempt.UnlockFn(); unlockErr != nil {
ctx.Log.Err("error unlocking state after plan error: %v", unlockErr)
}
if !lockFileExistsProirToPlan && common.FileExists(lockfilePath) {
// if the lock file did not exist before running the plan delete the lock file that was generated
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this only being run when there is an error in the steps? Does this file need to be present for an apply? Maybe it might make sense to use something like git to detect whether this is a new file that's been generated previously and clean this up before the init runs in the step runner itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot I have moved this outside the if err != nil {

@gezb gezb force-pushed the feature/fix_re-plan_regression branch from 391ce5d to 9275a76 Compare August 4, 2021 23:22
@gezb
Copy link
Contributor Author

gezb commented Aug 4, 2021

this still is not correct - Looking at the CircleCI output the apply step of the e2e tests now fails because the lock file does not exist - not sure of the best way to solve this now?

@nishkrishnan any ideas?

@nishkrishnan
Copy link
Contributor

this still is not correct - Looking at the CircleCI output the apply step of the e2e tests now fails because the lock file does not exist - not sure of the best way to solve this now?

@nishkrishnan any ideas?

I mentioned an idea in an above comment:

"Maybe it might make sense to use something like git cli to detect whether this is a new file that's been generated previously and clean this up before the init runs in the step runner itself."

If the file is committed, it won't show up when running git status.

@gezb gezb force-pushed the feature/fix_re-plan_regression branch from 9275a76 to 55e24e0 Compare August 7, 2021 23:04
@gezb
Copy link
Contributor Author

gezb commented Aug 7, 2021

OK I have rewritten this and I think its ready for re-review - One change I have had to make is require TF > 0.14.0 in the E2E tests as I have added a test around the lock file handling

@gezb gezb requested a review from nishkrishnan August 9, 2021 01:10
Copy link
Contributor

@nishkrishnan nishkrishnan left a comment

Choose a reason for hiding this comment

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

Nice! We're getting there!

Comment on lines 313 to 322
cmd := exec.Command("git", "ls-files", filename)
cmd.Dir = cloneDir

output, err := cmd.CombinedOutput()

if err != nil {
log.Warn("Error checking if %s is tracked: %s", filename, string(output))
return false, err
}
return len(output) > 0, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm more in favor with just keeping all this logic in core/runtime/common or even in InitStepRunner

  1. I dont know if there is really going to be something else that uses this logic. The long term solution is to just clean up after each operation
  2. ProjectCommandContext is becoming a dumping ground for things we need to plumb through the service and is losing it's meaning, let's not add to that unnecessarily.
  3. We can leave ProjectCommandRunner out of this, which already has a lot going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made the suggested change and moved the code to check if file is tracked in git into core/runtime/common

@gezb gezb force-pushed the feature/fix_re-plan_regression branch from f48a28c to 4e5a78f Compare August 18, 2021 18:28
@gezb gezb requested a review from nishkrishnan August 18, 2021 18:31
Copy link
Contributor

@nishkrishnan nishkrishnan left a comment

Choose a reason for hiding this comment

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

💯 Going to leave it open for a few days for other maintainers to look at before merging.

@nishkrishnan nishkrishnan merged commit 7ce8cef into runatlantis:master Aug 30, 2021
msarvar referenced this pull request in lyft/atlantis Sep 27, 2021
…aform init (#1701)

* Delete .terraform.lock.hcl prior to terraform init if it is not staged

* Changes suggeted by reviewer
krrrr38 pushed a commit to krrrr38/atlantis that referenced this pull request Dec 16, 2022
…aform init (runatlantis#1701)

* Delete .terraform.lock.hcl prior to terraform init if it is not staged

* Changes suggeted by reviewer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants