-
Notifications
You must be signed in to change notification settings - Fork 74
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
Exit with a success code instead of a panic when change set includes no change. #84
Exit with a success code instead of a panic when change set includes no change. #84
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this. I have a small suggestion for improvement
internal/aws/cfn/cfn.go
Outdated
@@ -261,7 +265,11 @@ func CreateChangeSet(template cft.Template, params []types.Parameter, tags map[s | |||
config.Debugf("ChangeSet status: %s", status) | |||
|
|||
if status == "FAILED" { | |||
return changeSetName, errors.New(ptr.ToString(res.StatusReason)) | |||
if ptr.ToString(res.StatusReason) == noChangeFoundMsg { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this logic adds any value here. Would prefer moving this into the deploy command to decide behaviour there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this feedback! I was just debating wheter I should add this logic into deploy.go or not. I'll modify it.
… or not into deploy.go.
internal/cmd/deploy/deploy.go
Outdated
@@ -159,7 +161,12 @@ YAML: | |||
spinner.Push("Creating change set") | |||
changeSetName, createErr := cfn.CreateChangeSet(template, parameters, combinedTags, stackName, roleArn) | |||
if createErr != nil { | |||
panic(ui.Errorf(createErr, "error creating changeset")) | |||
if createErr.Error() == noChangeFoundMsg { | |||
fmt.Println(console.Green("\nChange set was created, but there is no change. Deploy was skipped.")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you put a spinner.Pop()
just before this, you won't need the \n
at the beginning of the message :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a awesome utility!
https://github.com/aws-cloudformation/rain/blob/main/internal/console/spinner/spinner.go
I'll use this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
Issue #, if available: #73
Description of changes: When change set includes no change, rain exits with a success code instead of a panic.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.