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

Provide ability to disable backend init so that validate-all can be run without a backend #761

Merged
merged 4 commits into from
Jul 1, 2019

Conversation

stefansedich
Copy link
Contributor

@stefansedich stefansedich commented Jun 24, 2019

As an attempt to solve #597 and #732 this change provides the abilty to disable a backend using something like this, I set DISABLE_BACKEND in my CI process when running validate-all.

remote_state {
  backend = "s3"
  enabled = get_env("DISABLE_BACKEND", "false") != "true"
  ...
}

@yorinasub17 I ran with your idea but ended up in a slightly different direction, would appreicate some feedback on if this is close to what we want or if I should consider another solution as I would love to get this working so I can add an validate-all step to our CI process that does not require a backend.

@stefansedich stefansedich changed the title Provide ability to disable backend init Provide ability to disable backend init so that validate-all can be run without a backend Jun 24, 2019
config/config.go Outdated
@@ -520,10 +521,15 @@ func convertToTerragruntConfig(terragruntConfigFromFile *terragruntConfigFile, c
}

remoteState := &remote.RemoteState{}
remoteState.FillDefaults()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It appeared to me that FillDefaults should probably be called before setting any attributes? It was unused so was not fully sure of it's intention, I moved it up to execute earlier so that I could default Enabled to true when not present in config.

that the backend will not be initialized and -backend=false will be
passed to the terraform init command.
@stefansedich stefansedich force-pushed the disable-backend-init branch 2 times, most recently from f65fba3 to 6cd8793 Compare June 25, 2019 03:05
@stefansedich
Copy link
Contributor Author

I changed my original approach and moved from adding an attribute on remote_state and moving it up to the backend itself but am finding there is some test pain going around and setting Enabled = true around the place.

I will hold off doing that for now incase this approach is not how we want to tackle this issue.

@yorinasub17
Copy link
Contributor

I am a little confused by what is supposed to happen when enabled = false. Intuitively to me, this should mean that the remote state config should not be used at all in any terraform command. But it looks like the current implementation is to only disable all the initialization. Is that correct and intentional?

If so, I might rename the enabled var to enable_initialization or something like that so that it is clearer what is supposed to happen. Otherwise, this approach makes sense to me.

Would love a second opinion from one of the codeowners though: @autero1 @brikis98 @eak12913

@stefansedich
Copy link
Contributor Author

Agreed @yorinasub17 on second glance myself I can see the confusion and agree that enable_init might make more sense, I could even switch it to disable_init which would solve my issue of having to go and default things to true in all tests which should make things easier.

@stefansedich
Copy link
Contributor Author

@yorinasub17 I went ahead and moved to disable_init I think it makes much more sense!

@yorinasub17
Copy link
Contributor

Sorry for the radio silence here, got pulled away to other things. Did a quick review of the new things and it looks good except for the terragrunt binary that was checked in. Can you make a commit that removes that? Thanks!

Going to kick off the build now since that doesn't depend on that binary.

@stefansedich
Copy link
Contributor Author

Oops @yorinasub17 the binary has been removed!

@lorengordon
Copy link
Contributor

You might want to rebase/amend the commit that added the binary, instead. Or a maintainer can squash/rebase when merging. Otherwise the binary will end up in the repo commit history anyway...

@yorinasub17
Copy link
Contributor

@lorengordon Yup will squash when merging. The only reason why I wanted the additional commit was so that I can see the build status show up on 61f5345 . Otherwise, the commit squashing will create a new history and I will have to dig up the build status.

Looks like the build passed, so going to go ahead and merge + release this. Thanks for the contribution!

@yorinasub17 yorinasub17 merged commit 73a5b68 into gruntwork-io:master Jul 1, 2019
@yorinasub17
Copy link
Contributor

yorinasub17 commented Jul 1, 2019

Woops looks like there was a merge conflict that git failed to detect, due to the change in signature. Fixing it now...

@stefansedich
Copy link
Contributor Author

I hope I did not mess you up @yorinasub17, I must have pushed my rebase just as you clicked merge on your side.

@lorengordon
Copy link
Contributor

Sorry! I was trying to help keep the repo clean of binaries, but apparently you had it under control and I just made the merge more complicated! 🤦

@yorinasub17
Copy link
Contributor

No worries! The merge conflict wasn't actually because of the squashing or anything, but rather because of feature conflicts (the GCS initializer feature wasn't updated to match the new function signatures introduced here). These are harder to catch because they don't show up as conflicts in git.

I have a branch that fixes it here: #769

@stefansedich
Copy link
Contributor Author

Thanks @yorinasub17 my bad there I should have picked up on this too.

@yorinasub17
Copy link
Contributor

Released: https://github.com/gruntwork-io/terragrunt/releases/tag/v0.19.7

@cpxPratik
Copy link

Should not readme.md be updated? I do not see disable_init config option on readme file. Unless one look into source code, there is no way to know if that option is available.

@stefansedich
Copy link
Contributor Author

Good idea @cpxPratik I will add it to my list of things to tackle this week.

@yorinasub17
Copy link
Contributor

Good suggestion @cpxPratik ! That would be great if you can handle that @stefansedich . Looking forward to the PR!

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.

4 participants