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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to reorder var and var-file options #1217

Merged
merged 6 commits into from Dec 19, 2022
Merged

Add ability to reorder var and var-file options #1217

merged 6 commits into from Dec 19, 2022

Conversation

chilledornaments
Copy link
Contributor

Description

This adds a SetVarsAfterVarFiles field to the Options struct. When set to true, FormatArgs will place -var options after -var-file options. When false, FormatArgs will maintain its current behavior. This is a similar idea to #256.

This will increase terratest's flexibility without introducing a breaking changes 馃檪

TODOs

Read the Gruntwork contribution guidelines.

  • Update the docs.
  • Run the relevant tests successfully, including pre-commit checks.
  • Ensure any 3rd party code adheres with our license policy or delete this line if its not applicable.
  • Include release notes. If this PR is backward incompatible, include a migration guide.

Release Notes (draft)

Added ability to control order of -var and -var-file options passed to Terraform

Migration Guide

@denis256
Copy link
Member

Hi,
consecutive runs of TestFormatSetVarsAfterVarFilesFormatsCorrectly periodically fail with error on the assertion

=== CONT  TestFormatSetVarsAfterVarFilesFormatsCorrectly
    format_test.go:298: 
        	Error Trace:	format_test.go:298
        	Error:      	Not equal: 
        	            	expected: []string{"plan", "-var-file", "test.tfvars", "-var", "foo=bar", "-var", "hello=world", "-lock=false"}
        	            	actual  : []string{"plan", "-var-file", "test.tfvars", "-var", "hello=world", "-var", "foo=bar", "-lock=false"}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -5,5 +5,5 @@
        	            	  (string) (len=4) "-var",
        	            	+ (string) (len=11) "hello=world",
        	            	+ (string) (len=4) "-var",
        	            	  (string) (len=7) "foo=bar",
        	            	- (string) (len=4) "-var",
        	            	- (string) (len=11) "hello=world",
        	            	  (string) (len=11) "-lock=false"
        	Test:       	TestFormatSetVarsAfterVarFilesFormatsCorrectly
--- FAIL: TestFormatSetVarsAfterVarFilesFormatsCorrectly (0.00s)

@chilledornaments
Copy link
Contributor Author

HI @denis256,

Just pushed a commit to fix that! I didn't catch that failure locally because I hadn't cleaned my cache. Working locally now 馃檪

[14:40:01 ~/repos/chilledornaments/terratest/modules/terraform] > go clean -cache
[14:40:05 ~/repos/chilledornaments/terratest/modules/terraform] > go test -timeout 30s -run ^TestFormatSetVarsAfterVarFilesFormatsCorrectly$ 
PASS
ok      github.com/gruntwork-io/terratest/modules/terraform     0.275s

@denis256
Copy link
Member

Hello,
test case is still failing periodically:

=== CONT  TestFormatSetVarsAfterVarFilesFormatsCorrectly
    format_test.go:299: 
        	Error Trace:	format_test.go:299
        	Error:      	Not equal: 
        	            	expected: []string{"plan", "-var", "foo=bar", "-var", "hello=world", "-var-file", "test.tfvars", "-lock=false"}
        	            	actual  : []string{"plan", "-var", "hello=world", "-var", "foo=bar", "-var-file", "test.tfvars", "-lock=false"}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -3,5 +3,5 @@
        	            	  (string) (len=4) "-var",
        	            	+ (string) (len=11) "hello=world",
        	            	+ (string) (len=4) "-var",
        	            	  (string) (len=7) "foo=bar",
        	            	- (string) (len=4) "-var",
        	            	- (string) (len=11) "hello=world",
        	            	  (string) (len=9) "-var-file",
        	Test:       	TestFormatSetVarsAfterVarFilesFormatsCorrectly
--- FAIL: TestFormatSetVarsAfterVarFilesFormatsCorrectly (0.00s)


Expected :[]string{"plan", "-var", "foo=bar", "-var", "hello=world", "-var-file", "test.tfvars", "-lock=false"}
Actual   :[]string{"plan", "-var", "hello=world", "-var", "foo=bar", "-var-file", "test.tfvars", "-lock=false"}

I think should be implemented more smart verification than assert.Equal()

@chilledornaments
Copy link
Contributor Author

Hi @denis256 ,

I implemented logic in the test to use the checkResultWithRetry() function based on this comment. The test failures are caused by Go not caring about the order of keys when iterating over a map, not a problem with the changes to FormatArgs() (the order of -var and -var-file flags is always correct, but the order of -var arguments may change between runs, which is causing the test failures)

I was able to reproduce the test failures by running the test I added multiple times. I was also able to verify that the tests will pass given enough loops:

$ for i in {0..10}; do go clean -cache; go test -timeout 30s -run ^TestFormatSetVarsAfterVarFilesFormatsCorrectly$ -v ; done

[TRUNCATED] 

=== RUN   TestFormatSetVarsAfterVarFilesFormatsCorrectly
=== PAUSE TestFormatSetVarsAfterVarFilesFormatsCorrectly
=== CONT  TestFormatSetVarsAfterVarFilesFormatsCorrectly
--- PASS: TestFormatSetVarsAfterVarFilesFormatsCorrectly (0.00s)
PASS
ok      github.com/gruntwork-io/terratest/modules/terraform     0.266s
=== RUN   TestFormatSetVarsAfterVarFilesFormatsCorrectly
=== PAUSE TestFormatSetVarsAfterVarFilesFormatsCorrectly
=== CONT  TestFormatSetVarsAfterVarFilesFormatsCorrectly
    format_test.go:149: Retry 0 of FormatArgs(1) failed: expected [plan -var-file test.tfvars -var foo=bar -var hello=world -lock=false], got [plan -var-file test.tfvars -var hello=world -var foo=bar -lock=false]
    format_test.go:149: Retry 0 of FormatArgs(2) failed: expected [plan -var foo=bar -var hello=world -var-file test.tfvars -lock=false], got [plan -var hello=world -var foo=bar -var-file test.tfvars -lock=false]
--- PASS: TestFormatSetVarsAfterVarFilesFormatsCorrectly (0.00s)
PASS
ok      github.com/gruntwork-io/terratest/modules/terraform     0.249s
=== RUN   TestFormatSetVarsAfterVarFilesFormatsCorrectly
=== PAUSE TestFormatSetVarsAfterVarFilesFormatsCorrectly
=== CONT  TestFormatSetVarsAfterVarFilesFormatsCorrectly
--- PASS: TestFormatSetVarsAfterVarFilesFormatsCorrectly (0.00s)
PASS
ok      github.com/gruntwork-io/terratest/modules/terraform     0.275s

[TRUNCATED] 

@denis256
Copy link
Member

Hello,
approach using checkResultWithRetry is confusing, it makes to think that options formatting may not work each time.

A simplified approach may be to compare index of -var and -var-file arguments, which one should be before of others

@chilledornaments
Copy link
Contributor Author

Hi @denis256 ,

Thanks for the suggestion 馃檪 I've updated the test to do just that!

Copy link
Member

@denis256 denis256 left a comment

Choose a reason for hiding this comment

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

Failing tests aren't related to implemented changes

@denis256 denis256 merged commit e63181e into gruntwork-io:master Dec 19, 2022
estahn pushed a commit to estahn/k8s-image-swapper that referenced this pull request Jan 1, 2023
## [1.4.0](v1.3.3...v1.4.0) (2023-01-01)

### 馃懛 Build System

* **deps-dev:** Bump @semantic-release/changelog from 6.0.1 to 6.0.2 ([#404](#404)) ([ee56dbc](ee56dbc)), closes [#276](#276) [#276](#276) [#272](#272) [#275](#275) [#273](#273) [#274](#274) [#271](#271) [#270](#270) [#269](#269) [#268](#268) [#267](#267)

### 馃帀 Features

* add custom tags to created ECR repositories ([#191](#191)) ([9849df2](9849df2))

### 馃摑 Documentation

* fix indentation ([b00c57e](b00c57e))

### 猬嗭笍 Dependencies

* **deps:** Bump actions/cache from 3.0.11 to 3.2.1 ([#417](#417)) ([7e7eb8f](7e7eb8f)), closes [actions/cache#1039](actions/cache#1039) [actions/cache#1023](actions/cache#1023) [actions/cache#959](actions/cache#959) [actions/cache#960](actions/cache#960) [actions/cache#963](actions/cache#963) [actions/cache#961](actions/cache#961) [actions/cache#976](actions/cache#976) [actions/cache#971](actions/cache#971) [actions/cache#979](actions/cache#979) [actions/cache#986](actions/cache#986) [actions/cache#981](actions/cache#981) [actions/cache#997](actions/cache#997) [actions/cache#998](actions/cache#998) [actions/cache#1005](actions/cache#1005) [actions/cache#1007](actions/cache#1007) [actions/cache#1013](actions/cache#1013) [actions/cache#1004](actions/cache#1004) [actions/cache#1014](actions/cache#1014) [actions/cache#1008](actions/cache#1008) [actions/cache#1026](actions/cache#1026) [actions/cache#929](actions/cache#929) [actions/cache#1035](actions/cache#1035) [actions/cache#959](actions/cache#959) [actions/cache#979](actions/cache#979) [actions/cache#1013](actions/cache#1013) [actions/cache#1026](actions/cache#1026) [actions/cache#929](actions/cache#929) [actions/cache#1006](actions/cache#1006) [#1023](https://github.com/estahn/k8s-image-swapper/issues/1023) [#1039](https://github.com/estahn/k8s-image-swapper/issues/1039) [#1035](https://github.com/estahn/k8s-image-swapper/issues/1035) [#929](https://github.com/estahn/k8s-image-swapper/issues/929) [#1026](https://github.com/estahn/k8s-image-swapper/issues/1026) [#1008](https://github.com/estahn/k8s-image-swapper/issues/1008) [#1014](https://github.com/estahn/k8s-image-swapper/issues/1014) [#1004](https://github.com/estahn/k8s-image-swapper/issues/1004)
* **deps:** Bump actions/setup-python from 4.3.0 to 4.3.1 ([#406](#406)) ([16da762](16da762)), closes [actions/setup-python#559](actions/setup-python#559) [actions/setup-python#511](actions/setup-python#511) [actions/setup-python#558](actions/setup-python#558) [#559](#559) [#558](#558) [#549](#549) [#546](#546) [#545](#545) [#535](#535) [#510](#510) [#511](#511) [#520](#520)
* **deps:** Bump actions/setup-python from 4.3.1 to 4.4.0 ([#418](#418)) ([77872f8](77872f8)), closes [actions/setup-python#566](actions/setup-python#566) [#567](#567) [#569](#569) [#566](#566)
* **deps:** Bump github.com/aws/aws-sdk-go from 1.44.146 to 1.44.152 ([#403](#403)) ([9db51fd](9db51fd)), closes [#4652](https://github.com/estahn/k8s-image-swapper/issues/4652) [#4650](https://github.com/estahn/k8s-image-swapper/issues/4650) [#4648](https://github.com/estahn/k8s-image-swapper/issues/4648) [#4647](https://github.com/estahn/k8s-image-swapper/issues/4647) [#4646](https://github.com/estahn/k8s-image-swapper/issues/4646) [#4644](https://github.com/estahn/k8s-image-swapper/issues/4644) [#4639](https://github.com/estahn/k8s-image-swapper/issues/4639)
* **deps:** Bump github.com/aws/aws-sdk-go from 1.44.152 to 1.44.157 ([#411](#411)) ([2188432](2188432)), closes [#4658](https://github.com/estahn/k8s-image-swapper/issues/4658) [#4657](https://github.com/estahn/k8s-image-swapper/issues/4657) [#4656](https://github.com/estahn/k8s-image-swapper/issues/4656) [#4654](https://github.com/estahn/k8s-image-swapper/issues/4654) [#4653](https://github.com/estahn/k8s-image-swapper/issues/4653)
* **deps:** Bump github.com/aws/aws-sdk-go from 1.44.157 to 1.44.162 ([#415](#415)) ([f70fcd9](f70fcd9)), closes [#4666](https://github.com/estahn/k8s-image-swapper/issues/4666) [#4665](https://github.com/estahn/k8s-image-swapper/issues/4665) [#4663](https://github.com/estahn/k8s-image-swapper/issues/4663) [#4661](https://github.com/estahn/k8s-image-swapper/issues/4661) [#4660](https://github.com/estahn/k8s-image-swapper/issues/4660)
* **deps:** Bump github.com/aws/aws-sdk-go from 1.44.162 to 1.44.167 ([#419](#419)) ([f8b91fe](f8b91fe)), closes [#4671](https://github.com/estahn/k8s-image-swapper/issues/4671) [#4670](https://github.com/estahn/k8s-image-swapper/issues/4670) [#4669](https://github.com/estahn/k8s-image-swapper/issues/4669) [#4668](https://github.com/estahn/k8s-image-swapper/issues/4668) [#4667](https://github.com/estahn/k8s-image-swapper/issues/4667)
* **deps:** Bump github.com/gruntwork-io/terratest from 0.41.3 to 0.41.4 ([#402](#402)) ([16dde07](16dde07)), closes [#1208](https://github.com/estahn/k8s-image-swapper/issues/1208)
* **deps:** Bump github.com/gruntwork-io/terratest from 0.41.4 to 0.41.6 ([#409](#409)) ([9fc87df](9fc87df)), closes [#1214](https://github.com/estahn/k8s-image-swapper/issues/1214) [#1198](https://github.com/estahn/k8s-image-swapper/issues/1198)
* **deps:** Bump github.com/gruntwork-io/terratest from 0.41.6 to 0.41.7 ([#420](#420)) ([9ab97f2](9ab97f2)), closes [gruntwork-io/terratest#1217](gruntwork-io/terratest#1217) [#1217](https://github.com/estahn/k8s-image-swapper/issues/1217)
* **deps:** Bump goreleaser/goreleaser-action from 3.1.0 to 4.1.0 ([#414](#414)) ([e963ba1](e963ba1)), closes [goreleaser/goreleaser-action#382](goreleaser/goreleaser-action#382) [goreleaser/goreleaser-action#366](goreleaser/goreleaser-action#366) [goreleaser/goreleaser-action#379](goreleaser/goreleaser-action#379) [goreleaser/goreleaser-action#383](goreleaser/goreleaser-action#383) [goreleaser/goreleaser-action#366](goreleaser/goreleaser-action#366) [goreleaser/goreleaser-action#379](goreleaser/goreleaser-action#379) [goreleaser/goreleaser-action#370](goreleaser/goreleaser-action#370) [#374](#374) [#372](#372) [#373](#373) [#383](#383) [#366](#366) [#382](#382) [#370](#370)
* **deps:** Bump k8s.io/api from 0.25.4 to 0.26.0 ([#407](#407)) ([d13bf5e](d13bf5e)), closes [#111023](https://github.com/estahn/k8s-image-swapper/issues/111023) [#113375](https://github.com/estahn/k8s-image-swapper/issues/113375) [#113186](https://github.com/estahn/k8s-image-swapper/issues/113186)
* **deps:** Bump k8s.io/client-go from 0.25.4 to 0.26.0 ([#410](#410)) ([bcc56b5](bcc56b5)), closes [#113797](https://github.com/estahn/k8s-image-swapper/issues/113797) [#111023](https://github.com/estahn/k8s-image-swapper/issues/111023) [#113826](https://github.com/estahn/k8s-image-swapper/issues/113826) [#113375](https://github.com/estahn/k8s-image-swapper/issues/113375)
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