aws-sam-cli icon indicating copy to clipboard operation
aws-sam-cli copied to clipboard

Feature request: add hook(s) for cleaning up CF empty changesets

Open softprops opened this issue 2 years ago • 14 comments

Describe your idea/feature/enhancement

Provide a clear description. Ex. I wish SAM CLI would [...]

I wish SAM CLI would provide a sub command (and or transparent option) to have changesets which are ignored do to nothing changes which the user chooses ignore with --no-execute-changeset to clean up unapplied changesets

I've found in a few continuous deployment cases were I have --no-execute-changeset where I preview deployment with other applying as well as run deploy ignoring empty changesets with fail_on_empty_changeset set to false but these changesets accumulate and eventually lead to an error when the stack hits a cloud formation limit

I have a work around in the form of a custom script

for name in $(
  aws cloudformation list-change-sets --stack-name ${STACK_NAME}  --output text  --query "Summaries[?StatusReason=='The submitted information didn\'t contain changes. Submit different information to create a change set.'].ChangeSetName"
  ); do
 echo "deleting changeset $name"
 aws cloudformation delete-change-set --stack-name ${STACK_NAME}  --change-set-name $name;
done

I'd like something like this to be built-in and in the most ideal situations automatically delete an unapplied changeset when an deploy fails do to an empty changeset so I don't even have to think about it

Proposal

Add details of how to add this to the product.

either add a new subcommand do to this in masse, or an flag deploy to opt in or just do this automatically. I believe sam already knows when its an empty changeset error ie. the fail_on_empty_changeset hook.

Things to consider:

  1. Will this require any updates to the SAM Spec

nope

Additional Details

softprops avatar Aug 11 '22 18:08 softprops

@softprops Thanks for the feature request.

I think adding an option to auto delete the empty changesets, is probably the best way to go.

jfuss avatar Aug 12 '22 18:08 jfuss

With a bit of direction on what good looks like I might be able to manage adding this myself around here

softprops avatar Aug 13 '22 22:08 softprops

@softprops That would be awesome! I should have done that from the start, so my apologies on that.

I am doing my morning response to issues but will come back to this later today to give more direct links on areas that should be changed and more direction. (Going to assign to myself to help me get back here later)

jfuss avatar Aug 15 '22 14:08 jfuss

@softprops Sorry about the delay.

So what we would need to do is:

  1. agree upon a cli option name
  2. Add a click option/flag to the command here
  3. Vend that down the stack to here (I think)
  4. Write tests (unit and integration)

We will want to make sure deploy and deploy -guided are supported. We might want to make sure sync works with this too? But I don't think sync uses changesets to deploy but didn't verify that.

Was that helpful?

jfuss avatar Aug 16 '22 19:08 jfuss

I think adding an option to auto delete the empty changesets

I’d favor this option as well. Most sam users aren’t going to realize changesets that fail so to empty diffs are kept around by cf. I wasn’t and I thought I knew cf pretty well. All sam users are in the sam boat with respect to this hidden implementation detail creeping up on them and then when a deploy error happens, it’s degrees sams usability to have to debug the error.

if we do add a flag it would be for the less common or rare case where you want to keep cf change sets that are empty around. I don’t know what you would want to do with them if you did.

softprops avatar Aug 17 '22 12:08 softprops

@softprops I am not sure I follow. Adding a CLI option or Flag is almost the same thing. The difference is accepting a value (option) or boolean value (flag). Are you suggesting we should just delete it without adding CLI options or flags?

jfuss avatar Aug 17 '22 18:08 jfuss

Are you suggesting we should just delete it without adding CLI options or flags?

Yes, I think that's what users are likely expecting happens today but isn't. Let me know if that's a contentious idea.

it's only the case when those empty changestacks pile up that a user would discover they exist. I've ran into that a few times now so I'm become aware of the issue. I have a work around mentioned above but its a manual one as runbook with ci fails. Instead of that my wish would be that empty changesets aren't something I should have to worry about as a user.

What I mentioned above is kind of the reverse of what I think you were asking for. Have the auto delete behavior be the default and if sam has users which want empty changesets to accumulate in the stacks history, today's behavior, give them the option with a flag they can opt into to do so.

My personal opinion is that it would be a really rare case to want a stack to accumulate empty changesets either by default or by demand. It's an implementation detail I expect most to all sam users would not want to have to think about but today all are impacted by whether they've hit the limit or not.

softprops avatar Aug 17 '22 22:08 softprops

@softprops I agree. It's probably what we should have done from the beginning but I have some backwards compatibility concerns. Because we don't know all cases in which a customer may use or want the empty changeset around, adding in this behavior without a customer opt-ing in can be a breaking change to some customers. Most might not care but for those who might, it breaks our compatibility.

So the easiest way forward it to introduce a way for customers to opt into this new behavior. However, I will bring it up with the team, as maybe this one is worth it. If it is, we would really have to dive into if there are cases we would be breaking and try to notify those customers (which is hard).

jfuss avatar Aug 18 '22 15:08 jfuss

@softprops I chatted with the team. Due to the uncertainty and risk of what we could break, the team would prefer in the opt-in way through a command line option or flag with the v1.X scope of SAM CLI. With the samconfig.toml, this at least allows customers to specific it (once per project) which lowers some cost. We can also log to console within SAM CLI, to help bring attention. We can then revisit this within a SAM CLI v2 context (whenever that is) to move to your proposal and ideal solution.

Are you still willing to contribute a patch for this?

jfuss avatar Aug 24 '22 17:08 jfuss

We also encountered this issue. See related issue: https://github.com/aws/aws-cli/issues/4534

hoffa avatar Mar 21 '23 16:03 hoffa

@jfuss my company keeps bumping into this, breaking automation pipelines and then reactively running semi hacky scripts to manage garbage collecting the accumulated empty changesets to unblock them.

I’d like to start spit balling some flag names for to keep the discussion going

We use fail-on-empty-changeset set to false in samconfig files.

What do you think of the flag name delete-empty-changset, defaulting to false to be backwards compatible with todays behavior?

softprops avatar Aug 22 '23 23:08 softprops

Did anything ever come of this? I'm also starting to run into the same issue.

mew1033 avatar Feb 23 '24 21:02 mew1033