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

fix for handling -no-color and -var options #933

Merged
merged 1 commit into from
Jun 21, 2021
Merged

fix for handling -no-color and -var options #933

merged 1 commit into from
Jun 21, 2021

Conversation

tolgaio
Copy link
Contributor

@tolgaio tolgaio commented Jun 17, 2021 •
edited
Loading

This PR fixes a few things:

  • terraform apply expects plan file to be presented as the last
    argument, currently -no-color option is added after the plan file in the argument/options order.
  • when terraform apply is provided with a plan file, we should not pass
    -var and -var-file options anymore. it is a violation.

brikis98
brikis98 reviewed Jun 17, 2021
View reviewed changes
Copy link
Member

@brikis98 brikis98 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 the PR! This looks like a good fix. I had two tiny NITs to consider, and then I can kick off tests.

modules/terraform/apply_test.go Outdated Show resolved Hide resolved
modules/terraform/format.go Outdated Show resolved Hide resolved
@tolgaio tolgaio requested a review from brikis98 June 17, 2021 10:26
brikis98
brikis98 previously approved these changes Jun 18, 2021
View reviewed changes
Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you! I'll kick off tests now.

@tolgaio
Copy link
Contributor Author

tolgaio commented Jun 19, 2021

hi @brikis98 seems like there are some flaky tests failing in the CI, which I can reproduce locally as well. shall we merge this PR now and address those flaky ones separately?

@tolgaio
Copy link
Contributor Author

tolgaio commented Jun 19, 2021 •
edited
Loading

turns out that not all the tests are flaky, just realised some of the terraform validation tests have been fixed in the master. I've rebased mine from the latest master, it should pass the tests now, Could you run tests again @brikis98, thx.

@tolgaio tolgaio requested a review from brikis98 June 19, 2021 18:04
@tolgaio
fix for handling -no-color and -var options
8969ed9 
`terraform apply` expects `plan file` to be presented as the last
argument, currently -no-color option comes after the plan file. this
commit fixes that.

when `terraform apply` is provided with a plan file, we should not pass
`-var` and `-var-file` options anymore. it is a violation. this commit
fixes that.
brikis98
brikis98 approved these changes Jun 21, 2021
View reviewed changes
Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

Thanks! I'll kick tests off again.

@tolgaio
Copy link
Contributor Author

tolgaio commented Jun 21, 2021 •
edited
Loading

@brikis98 only this test is failing TestDoInBackgroundUntilStopped - retry. I haven't been able to re-produce it locally, could that be something flaky?

@tolgaio
Copy link
Contributor Author

tolgaio commented Jun 21, 2021

local:

=== RUN   TestDoInBackgroundUntilStopped
=== PAUSE TestDoInBackgroundUntilStopped
=== CONT  TestDoInBackgroundUntilStopped
TestDoInBackgroundUntilStopped 2021-06-21T16:53:46+01:00 retry.go:170: Executing action 'TestDoInBackgroundUntilStopped'
TestDoInBackgroundUntilStopped 2021-06-21T16:53:46+01:00 retry.go:174: Sleeping for 5s before repeating action 'TestDoInBackgroundUntilStopped'
TestDoInBackgroundUntilStopped 2021-06-21T16:53:51+01:00 retry.go:170: Executing action 'TestDoInBackgroundUntilStopped'
TestDoInBackgroundUntilStopped 2021-06-21T16:53:51+01:00 retry.go:174: Sleeping for 5s before repeating action 'TestDoInBackgroundUntilStopped'
TestDoInBackgroundUntilStopped 2021-06-21T16:53:56+01:00 retry.go:170: Executing action 'TestDoInBackgroundUntilStopped'
TestDoInBackgroundUntilStopped 2021-06-21T16:53:56+01:00 retry.go:174: Sleeping for 5s before repeating action 'TestDoInBackgroundUntilStopped'
TestDoInBackgroundUntilStopped 2021-06-21T16:54:01+01:00 retry.go:180: Received stop signal for action 'TestDoInBackgroundUntilStopped'.
--- PASS: TestDoInBackgroundUntilStopped (30.00s)
PASS
ok      github.com/gruntwork-io/terratest/modules/retry 33.696s

@brikis98
Copy link
Member

Yea, you're right, that looks like a flaky test. Must be a timing thing with concurrency in CircleCi. I've filed #936 to track it. That needs to be fixed separately, but it shouldn't block this PR, so I'm going to merge. Thanks!

@brikis98 brikis98 merged commit 7e38458 into gruntwork-io:master Jun 21, 2021
@brikis98
Copy link
Member

https://github.com/gruntwork-io/terratest/releases/tag/v0.36.1

This was referenced Jun 21, 2021
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brikis98 brikis98 brikis98 approved these changes

@yorinasub17 yorinasub17 Awaiting requested review from yorinasub17

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tolgaio @brikis98