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

read_terragrunt_config #1051

Merged
merged 7 commits into from Feb 18, 2020
Merged

read_terragrunt_config #1051

merged 7 commits into from Feb 18, 2020

Conversation

yorinasub17
Copy link
Contributor

@yorinasub17 yorinasub17 commented Feb 14, 2020

This implements read_terragrunt_config, a helper function that can be used for reading in another terragrunt config and accessing its blocks and attributes for reuse.

This is one of the suggested improvements coming out of #1025

TODO

  • Update when Codegen #1050 merges (there are some conflicts in that generate blocks will need to be added to the converter).

Copy link
Member

@brikis98 brikis98 left a comment

Great work Yori!

// Specifically, we want to reference blocks by named attributes, but blocks are rendered to lists in the
// TerragruntConfig struct, so we need to do some massaging of the data to convert the list of blocks in to a map going
// from the block name label to the block value.
func terragruntConfigAsCty(config *TerragruntConfig) (cty.Value, error) {
Copy link
Member

@brikis98 brikis98 Feb 14, 2020

Choose a reason for hiding this comment

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

Ooof, Terragrunt config parsing gets worse and worse... So much manual stuff to massage between formats.

Is there any way to add a test or lint check or something that ensures every new parameter we add to TerragruntConfig is handled here, and if we forget one, we at least get a test failure warning us of that?

Copy link
Contributor Author

@yorinasub17 yorinasub17 Feb 16, 2020

Choose a reason for hiding this comment

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

Yea I was not very happy with this... but I also couldn't come up with a way to avoid it. I'll see if I can add a test here.

Copy link
Contributor Author

@yorinasub17 yorinasub17 Feb 16, 2020

Choose a reason for hiding this comment

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

Added some unit tests: c5ea857

Copy link
Member

@brikis98 brikis98 Feb 16, 2020

Choose a reason for hiding this comment

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

This test does a great job of checking that all the existing fields are converted, but if we were to add a new field, and forget to to add it to this test and to terragruntConfigAsCty... The test won't fail, right?

Copy link
Contributor Author

@yorinasub17 yorinasub17 Feb 18, 2020

Choose a reason for hiding this comment

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

Hmm my understanding is that it should fail here, where it will exhaust the options when it sees a new struct field.

I'm going to verify this now that I merged in the codegen feature. My expectation is that the test will fail when I rebase and push. If it doesn't, I'll update the test until it fails.

Copy link
Contributor Author

@yorinasub17 yorinasub17 Feb 18, 2020

Choose a reason for hiding this comment

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

Yup failed as expected: https://circleci.com/gh/gruntwork-io/terragrunt/4101

=== RUN   TestTerragruntConfigAsCtyDrift
--- FAIL: TestTerragruntConfigAsCtyDrift (0.00s)
    config_as_cty_test.go:178: Unknown struct property: GenerateConfigs

Copy link
Member

@brikis98 brikis98 Feb 18, 2020

Choose a reason for hiding this comment

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

Ah, nice!

config/config_helpers.go Show resolved Hide resolved
@yorinasub17 yorinasub17 requested a review from brikis98 Feb 16, 2020
@yorinasub17
Copy link
Contributor Author

yorinasub17 commented Feb 16, 2020

I believe I addressed all the comments, but given the complexity of the new tests, I rerequested a review.

Copy link
Member

@brikis98 brikis98 left a comment

It LGTM overall. My only concern is about the maintainability of terragruntConfigAsCty... Or more accurately, of the 3-4 places we now have to update every time we add a new config option to Terragrunt. Sadly, Go is a bit limit here, so I don't have any suggestions on how to improve this 😞

@yorinasub17
Copy link
Contributor Author

yorinasub17 commented Feb 18, 2020

My only concern is about the maintainability of terragruntConfigAsCty... Or more accurately, of the 3-4 places we now have to update every time we add a new config option to Terragrunt.

Filed #1055 so we don't forget this thought.

@yorinasub17
Copy link
Contributor Author

yorinasub17 commented Feb 18, 2020

UPDATE: bfeab4c

Added generate block and property to the output cty and updated the tests so that they pass and check that the block and property is accessible.

I also found a bug in the generate block where comment_prefix was ignored, so fixed that.

Thanks for the review! Going to merge this in now!

@yorinasub17 yorinasub17 merged commit ac89420 into master Feb 18, 2020
@yorinasub17 yorinasub17 deleted the yori-read-terragrunt-config branch Feb 18, 2020
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