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

Forward SIGTERM signal to terraform #72

Merged
merged 1 commit into from
Jan 3, 2017
Merged

Forward SIGTERM signal to terraform #72

merged 1 commit into from
Jan 3, 2017

Conversation

mildred
Copy link
Contributor

@mildred mildred commented Dec 13, 2016

For #61

@mildred
Copy link
Contributor Author

mildred commented Dec 13, 2016

Merged appropriate changes in terraform, see hashicorp/terraform@e9c35ea

brikis98
brikis98 requested changes Dec 14, 2016
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! I left a bunch of comments.

Could you run the tests and paste the output here?

shell/run_shell_cmd.go
select {
case s := <-c:
if s == nil {
return
Copy link
Member

Choose a reason for hiding this comment

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

Is this a normal case or indicate of an odd problem? Perhaps it should be logged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the case when the channel is closed

shell/run_shell_cmd.go
}
}
}()

Copy link
Member

Choose a reason for hiding this comment

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

Could you extract this code into a separate function with a clear name? Perhaps forwardSignals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in new commit

shell/run_shell_cmd.go
@@ -22,5 +23,26 @@ func RunShellCommand(terragruntOptions *options.TerragruntOptions, command strin

cmd.Dir = terragruntOptions.WorkingDir

c := make(chan os.Signal, 1)
Copy link
Member

Choose a reason for hiding this comment

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

Please use a more descriptive variable name than c. Perhaps signalChannel.

shell/run_shell_cmd.go
terragruntOptions.Logger.Printf("Forward signal %s to terraform.", s.String())
err := cmd.Process.Signal(s)
if err != nil {
terragruntOptions.Logger.Println(err)
Copy link
Member

Choose a reason for hiding this comment

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

Might want a slightly clearer error message, such as terragruntOptions.Logger.Printf("Error forwarding signal: %v", err).

shell/run_shell_cmd.go
@@ -22,5 +23,26 @@ func RunShellCommand(terragruntOptions *options.TerragruntOptions, command strin

cmd.Dir = terragruntOptions.WorkingDir

c := make(chan os.Signal, 1)
signal.Notify(c, forwardSignals...)
Copy link
Member

Choose a reason for hiding this comment

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

It's worth mentioning that there is already code listening on some signals in Terragrunt here: https://github.com/gruntwork-io/terragrunt/blob/master/locks/lock.go#L49

Is it OK to listen in more than one place? Should the two be combined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the documentation explicitly says it's ok: https://golang.org/pkg/os/signal/#Notify

It is allowed to call Notify multiple times with different channels and the same signals: each channel receives copies of incoming signals independently.

@mildred
Copy link
Contributor Author

mildred commented Dec 14, 2016

I updated the code with your remarks.

The tests are failing, But the parent commit does not pass them either. There are failures related to cli (which I didn't changed) and other changes failing because I can't access a S3 bucket.

?   	github.com/gruntwork-io/terragrunt	[no test files]
?   	github.com/gruntwork-io/terragrunt/aws_helper	[no test files]
ok  	github.com/gruntwork-io/terragrunt/cli	0.004s
--- FAIL: TestResolveTerragruntConfigString (0.00s)
        Error Trace:    config_helpers_test.go:313
	Error:		Should be true
	Messages:	For string '${find_in_parent_folders()}' include <nil> and options {../test/fixture-parent-folders/no-terragrunt-in-root/child/sub-child/.terragrunt true []  <nil> <nil>}, expected error Could not find a .terragrunt config file in any of the parent folders of ../test/fixture-parent-folders/no-terragrunt-in-root/child/sub-child/.terragrunt but got error <nil>
		
--- FAIL: TestResolveTerragruntInterpolation (0.00s)
        Error Trace:    config_helpers_test.go:199
	Error:		Should be true
	Messages:	For string '${find_in_parent_folders()}' include <nil> and options {../test/fixture-parent-folders/no-terragrunt-in-root/child/sub-child/.terragrunt true []  <nil> <nil>}, expected error Could not find a .terragrunt config file in any of the parent folders of ../test/fixture-parent-folders/no-terragrunt-in-root/child/sub-child/.terragrunt but got error <nil>
		
--- FAIL: TestFindInParentFolders (0.00s)
        Error Trace:    config_helpers_test.go:120
	Error:		Should be true
	Messages:	For options {../test/fixture-parent-folders/no-terragrunt-in-root/child/sub-child/.terragrunt true []  <nil> <nil>}, expected error Could not find a .terragrunt config file in any of the parent folders of ../test/fixture-parent-folders/no-terragrunt-in-root/child/sub-child/.terragrunt but got error <nil>
		
FAIL
FAIL	github.com/gruntwork-io/terragrunt/config	0.009s
?   	github.com/gruntwork-io/terragrunt/errors	[no test files]
ok  	github.com/gruntwork-io/terragrunt/locks	0.006s
ok  	github.com/gruntwork-io/terragrunt/locks/dynamodb	55.844s
?   	github.com/gruntwork-io/terragrunt/options	[no test files]
ok  	github.com/gruntwork-io/terragrunt/remote	0.006s
?   	github.com/gruntwork-io/terragrunt/shell	[no test files]
ok  	github.com/gruntwork-io/terragrunt/spin	0.029s
[terragrunt] 2016/12/14 09:37:14 Reading Terragrunt config file at fixture-lock/.terragrunt
[terragrunt] 2016/12/14 09:37:14 Reading Terragrunt config file at /tmp/terragrunt-parent-child-test068407343/qa/my-app/.terragrunt
[terragrunt]  Are you sure you want to acquire a long-term lock? (y/n) 
[terragrunt] 2016/12/14 09:37:14 The non-interactive flag is set to true, so assuming 'yes' for all prompts
[terragrunt] 2016/12/14 09:37:14 Acquiring long-term lock. To release the lock, use the release-lock command.
[terragrunt] 2016/12/14 09:37:14 Attempting to acquire lock for state file terragrunt-test-fixture-lock in DynamoDB
[terragrunt] 2016/12/14 09:37:14 Reading Terragrunt config file at /tmp/.terragrunt868527042
[terragrunt] 2016/12/14 09:37:14 Initializing remote state for the s3 backend
[terragrunt] 2016/12/14 09:37:14 Initializing remote state for the s3 backend
[terragrunt] 2016/12/14 09:37:14 Stack at /tmp/terragrunt-stack-test821366288/fixture-stack/mgmt:
  => Module /tmp/terragrunt-stack-test821366288/fixture-stack/mgmt/vpc (dependencies: [])
  => Module /tmp/terragrunt-stack-test821366288/fixture-stack/mgmt/bastion-host (dependencies: [/tmp/terragrunt-stack-test821366288/fixture-stack/mgmt/vpc])
[terragrunt]  Are you sure you want to run 'terragrunt apply' in each folder of the stack described above? (y/n) 
[terragrunt] 2016/12/14 09:37:14 The non-interactive flag is set to true, so assuming 'yes' for all prompts
[terragrunt] [/tmp/terragrunt-stack-test821366288/fixture-stack/mgmt/bastion-host] 2016/12/14 09:37:14 Module /tmp/terragrunt-stack-test821366288/fixture-stack/mgmt/bastion-host must wait for 1 dependencies to finish
[terragrunt] [/tmp/terragrunt-stack-test821366288/fixture-stack/mgmt/vpc] 2016/12/14 09:37:14 Module /tmp/terragrunt-stack-test821366288/fixture-stack/mgmt/vpc must wait for 0 dependencies to finish
[terragrunt] [/tmp/terragrunt-stack-test821366288/fixture-stack/mgmt/vpc] 2016/12/14 09:37:14 Running module /tmp/terragrunt-stack-test821366288/fixture-stack/mgmt/vpc now
[terragrunt] [/tmp/terragrunt-stack-test821366288/fixture-stack/mgmt/vpc] 2016/12/14 09:37:14 Reading Terragrunt config file at /tmp/terragrunt-stack-test821366288/fixture-stack/mgmt/vpc/.terragrunt
[terragrunt] [/tmp/terragrunt-stack-test821366288/fixture-stack/mgmt/vpc] 2016/12/14 09:37:14 Initializing remote state for the s3 backend
[terragrunt] 2016/12/14 09:37:14 Attempting to create lock item for state file terragrunt-test-fixture-lock in DynamoDB table terragrunt_locks
[terragrunt] 2016/12/14 09:37:15 Lock acquired!
[terragrunt] 2016/12/14 09:37:15 Reading Terragrunt config file at fixture-lock/.terragrunt
[terragrunt] 2016/12/14 09:37:15 Attempting to acquire lock for state file terragrunt-test-fixture-lock in DynamoDB
[terragrunt]  Remote state S3 bucket terragrunt-test-bucket-oida0n does not exist or you don't have permissions to access it. Would you like Terragrunt to create it? (y/n) 
[terragrunt] 2016/12/14 09:37:15 The non-interactive flag is set to true, so assuming 'yes' for all prompts
[terragrunt] 2016/12/14 09:37:15 Creating S3 bucket terragrunt-test-bucket-oida0n
[terragrunt] 2016/12/14 09:37:15 Attempting to create lock item for state file terragrunt-test-fixture-lock in DynamoDB table terragrunt_locks
[terragrunt]  Remote state S3 bucket terragrunt-test-bucket-mtamit does not exist or you don't have permissions to access it. Would you like Terragrunt to create it? (y/n) [terragrunt] [/tmp/terragrunt-stack-test821366288/fixture-stack/mgmt/vpc]  Remote state S3 bucket terragrunt-test-bucket-8xh4lv does not exist or you don't have permissions to access it. Would you like Terragrunt to create it? (y/n) 

[terragrunt] 2016/12/14 09:37:15 The non-interactive flag is set to true, so assuming 'yes' for all prompts
[terragrunt] [/tmp/terragrunt-stack-test821366288/fixture-stack/mgmt/vpc] 2016/12/14 09:37:15 The non-interactive flag is set to true, so assuming 'yes' for all prompts
[terragrunt] 2016/12/14 09:37:15 Creating S3 bucket terragrunt-test-bucket-mtamit
[terragrunt] [/tmp/terragrunt-stack-test821366288/fixture-stack/mgmt/vpc] 2016/12/14 09:37:15 Creating S3 bucket terragrunt-test-bucket-8xh4lv
[terragrunt] 2016/12/14 09:37:16 Someone already has a lock on state file terragrunt-test-fixture-lock! arn:aws:iam::095348363195:user/terraform@192.168.1.11 acquired the lock on 2016-12-14 08:37:15.315430754 +0000 UTC.
Unable to acquire lock for item terragrunt-test-fixture-lock after 1 retries.
FAIL	github.com/gruntwork-io/terragrunt/test	1.713s
ok  	github.com/gruntwork-io/terragrunt/util	0.015s

@brikis98
Copy link
Member

The S3 bucket thing is expected. The code actually creates it for you.

The other three errors are probably a silly bug. Out of curiosity, does one of the parent folders of where you have the terragrunt checked out happen to contain a .terragrunt file?

@mildred
Copy link
Contributor Author

mildred commented Jan 3, 2017

None of the parent folders contains a .terragrunt file: In the subfolders the list of .terragrunt files is in the test directory:

./test/fixture/.terragrunt
./test/fixture-include/.terragrunt
./test/fixture-include/qa/my-app/.terragrunt
./test/fixture-lock/.terragrunt
./test/fixture-parent-folders/multiple-terragrunt-in-parents/.terragrunt
./test/fixture-parent-folders/multiple-terragrunt-in-parents/child/.terragrunt
./test/fixture-parent-folders/multiple-terragrunt-in-parents/child/sub-child/.terragrunt
./test/fixture-parent-folders/multiple-terragrunt-in-parents/child/sub-child/sub-sub-child/.terragrunt
./test/fixture-parent-folders/no-terragrunt-in-root/child/sub-child/.terragrunt
./test/fixture-parent-folders/terragrunt-in-root/.terragrunt
./test/fixture-parent-folders/terragrunt-in-root/child/sub-child/sub-sub-child/.terragrunt

@brikis98
Copy link
Member

brikis98 commented Jan 3, 2017

Hm, AFAIK, tests have been passing consistently in CI and other people's computers recently, so I'm a bit stumped why they would be failing on yours. It seems unlikely to be related to your change, so I'm going to try to merge it in.

@brikis98 brikis98 merged commit a8bb49e into gruntwork-io:master Jan 3, 2017
@brikis98
Copy link
Member

brikis98 commented Jan 3, 2017

Looks like the tests passed. I just created a new release and the binaries should be there in a few minutes: https://github.com/gruntwork-io/terragrunt/releases/tag/v0.7.3. Thanks again for the PR!

@brikis98 brikis98 mentioned this pull request Jan 8, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brikis98 brikis98 brikis98 requested changes

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@mildred @brikis98