Skip to content

Fix issue: Generated backend.tf "region" place is random #1102 #1165

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

Merged
merged 11 commits into from
Jan 12, 2021

Conversation

Jerazol
Copy link
Contributor

@Jerazol Jerazol commented May 6, 2020

No description provided.

Copy link
Contributor

@yorinasub17 yorinasub17 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 your contribution! This looks good to me! Will kick off a build to give you one feedback cycle, but can you also add a regression test? I'm thinking taking one of the generator tests, duplicating it, and running it in a tight loop multiple times to verify it always generates the same file.

@RykHawthorn
Copy link

#1102

@NasAmin
Copy link

NasAmin commented Dec 29, 2020

I'll really appreciate if someone can give this PR 👀. It will solve a small but very annoying problem.

@yorinasub17
Copy link
Contributor

It looks like there hasn't been an update to add a regression test yet (as requested in my comment). If anyone has interest to continue this PR to add a regression test, I can pull this fork into a branch into this repo so you can get it on your forks to commit on top of it.

@jonathansp
Copy link

yorinasub17

please have a look. It adds a few test cases to ensure "region" is not being placed in random order.

tests: add test coverage for RemoteStateConfigToTerraformCode gruntwork-io#1165
@jonathansp
Copy link

jonathansp commented Jan 11, 2021

It looks like there hasn't been an update to add a regression test yet (as requested in my comment). If anyone has interest to continue this PR to add a regression test, I can pull this fork into a branch into this repo so you can get it on your forks to commit on top of it.

Tests added @yorinasub17 @brikis98

Copy link
Contributor

@yorinasub17 yorinasub17 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 extending with test cases! The tests look pretty good, but had a few minor issues. Once those are resolved, I can kick off a build!

"c": 3,
},
expectedOrdered,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

golang maps are unordered when they are stored, so changing the order in the call here has no effect. Drop one of the test cases here, and instead update the test routine to call the function N times (20 like the other one?).

Choose a reason for hiding this comment

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

Changed.

expected, _ := ioutil.ReadFile(filepath.Join(generateTestCase, "backend.tf"))
for i := 1; i < 20; i++ {
runTerragruntCommand(t, fmt.Sprintf("terragrunt output -no-color -json --terragrunt-non-interactive --terragrunt-working-dir %s", generateTestCase), &stdout, &stderr)
actual, _ := ioutil.ReadFile(filepath.Join(generateTestCase, "backend.tf"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Check errors here instead of throwing them away (with require.NoError). Need to do this for the call above as well.

Choose a reason for hiding this comment

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

Perfect, will change.


runTerragruntCommand(t, fmt.Sprintf("terragrunt output -no-color -json --terragrunt-non-interactive --terragrunt-working-dir %s", generateTestCase), &stdout, &stderr)
expected, _ := ioutil.ReadFile(filepath.Join(generateTestCase, "backend.tf"))
for i := 1; i < 20; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use 0-indexing:

Suggested change
for i := 1; i < 20; i++ {
for i := 0; i < 20; i++ {

Choose a reason for hiding this comment

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

fixed.

@yorinasub17 yorinasub17 self-assigned this Jan 12, 2021
Jonathan Prates and others added 2 commits January 12, 2021 17:02
Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

Updates LGTM! Going to kick off a build, and if it passes, will merge this in!

Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

Build passed, so will go through to merge! Thanks for your contribution!

@yorinasub17 yorinasub17 merged commit da64227 into gruntwork-io:master Jan 12, 2021
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.

5 participants