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

"cdk diff" returns error code 1 #2111

Closed
xapou opened this issue Mar 28, 2019 · 12 comments · Fixed by #4650
Closed

"cdk diff" returns error code 1 #2111

xapou opened this issue Mar 28, 2019 · 12 comments · Fixed by #4650
Assignees
Labels
feature-request A feature should be added or improved. package/tools Related to AWS CDK Tools or CLI

Comments

@xapou
Copy link

xapou commented Mar 28, 2019

Calling cdk difffrom NPM fails because the command does not return 0.

package.json

  "scripts": {
    "diff": "cdk diff"    
  }

Running npm run diff fails.

cdk diff; echo $?shows the error code returned is 1.

@xapou
Copy link
Author

xapou commented Mar 28, 2019

A workaround to get rid of the `npm ERR! ...:

package.json

  "scripts": {
    "diff": "cdk diff || true"    
  }

@Doug-AWS
Copy link
Contributor

Is this enough of an issue to document? Or are we going to try to make it return 0?

@jogold
Copy link
Contributor

jogold commented Mar 28, 2019

@eladb
Copy link
Contributor

eladb commented Mar 28, 2019

As much as this is a nice a pure way to express that "there is a diff". I doesn't seem like that's what people expect. This is the 3rd or 4th time someone fell into this little pit. I'd argue that we can add a flag --fail for those brave souls who are interested to use cdk diff to check if there is a diff and change the default behavior to what people most expect.

@isubuz
Copy link

isubuz commented Aug 23, 2019

For scripts which rely on the error code from cdk diff, there is no way to know if this is due to diffs or some other error (e.g. invalid python code). The flag seems quite appropriate. This is how git does it as well.

git diff --name-only
echo $? # returns 0 

git diff --name-only --exit-code
echo $? # returns 1 if there are diffs

@rix0rrr rix0rrr added the feature-request A feature should be added or improved. label Aug 29, 2019
@shivlaks shivlaks added the package/tools Related to AWS CDK Tools or CLI label Aug 29, 2019
@abelmokadem
Copy link
Contributor

Any updates?

@jogold
Copy link
Contributor

jogold commented Oct 23, 2019

Any updates?

Will open a PR for this.

jogold added a commit to jogold/aws-cdk that referenced this issue Oct 23, 2019
Add the `--fail` to exit with 1 if there's a diff.

Closes aws#2111

BREAKING CHANGE: `cdk diff` now exits with 0 even when there's a diff, use `--fail` to exit with 1.
jogold added a commit to jogold/aws-cdk that referenced this issue Oct 23, 2019
Add the `--fail` flag to exit with 1 if there's a diff.

Closes aws#2111

BREAKING CHANGE: `cdk diff` now exits with 0 even when there's a diff, use `--fail` to exit with 1.
@mergify mergify bot closed this as completed in #4650 Oct 23, 2019
mergify bot pushed a commit that referenced this issue Oct 23, 2019
Add the `--fail` flag to exit with 1 if there's a diff.

Closes #2111

BREAKING CHANGE: `cdk diff` now exits with 0 even when there's a diff, use `--fail` to exit with 1.
@timwelch
Copy link

timwelch commented Jan 5, 2021

When running through a pipeline (i.e. Jenkins), I'd actually expect a different result. If there's a diff that's cool and I want to move on to the next stage of the pipeline and deploy it, so an exit code of 1 is actually not good here. If there's an ERROR, like someone committed bad code to the repo which causes cdk diff to actually throw an error, like invalid syntax or anything that would cause it to NOT deploy... That seems like an error code of 1 or 2 or something else, no?

Expected results would be:

exit 0 -- No diff and the expected pre-compilation or whatever didn't find any errors in synthesizing.
exit 1 -- You have an error, I could not synth due to any number of reasons, invalid syntax, etc etc.
exit 2 -- Synth worked, code looks clean, and there is a diff that I would apply if you were to deploy.

With those, I can create really functional pipeline steps. With exit 0, I can skip the deploy stage entirely because there's no need. With exit 1, I can shoot an email to the Dev who last committed to the repo and broke the pipeline with bad syntax or something. With exit 2, I know something changed and I move on to apply it.

With either a blanket return of 0, or forcing me now to use "|| true" ... I actually get nothing. I end up having to create an Approval stage where the pipeline sits until someone looks at the output of the Diff to see if it was a real change or if the CDK is now broken and wouldn't deploy if we wanted it to.

Suggestions / Thoughts?

@abelmokadem
Copy link
Contributor

@timwelch what if there was no CloudFormation diff, but you updated the CloudFormation tags? I think that's the only scenario I see that isn't covered in what you mentioned.

@timwelch
Copy link

timwelch commented Jan 6, 2021

I didn't know the tags was an issue, but I also don't know the full inner workings of cdk and/or cloudformation. It actually sounds like another instance CDK / CFN not showing any changes are needed.

A key example I have of that is in an ASG, when you modify the update_policy....

        self.asg = autoscaling.AutoScalingGroup(
            self,
            rekor.id_str(self.app_name+'-ASG'),
            role=self.role,
            vpc=self.vpc,
            instance_type=ec2.InstanceType(self.env_data['instance_type']),
            machine_image=self.ami,
            vpc_subnets=self.private_subnets,
            # desired_capacity=0,
            max_capacity=2,
            # min_capacity=0,
            user_data=self.userdata,
            key_name=self.keypair.name,
            associate_public_ip_address=False,
            security_group=self.sg,
            cooldown=core.Duration.seconds(180),
            ignore_unmodified_size_properties=True,
            notifications=None if not self.notifications_sns_topic else [
                autoscaling.NotificationConfiguration(
                    topic=self.notifications_sns_topic,
                    scaling_events=autoscaling.ScalingEvents.ALL)],
            update_policy=autoscaling.UpdatePolicy.rolling_update(
                max_batch_size=1,
                min_instances_in_service=1,
                suspend_processes=[
                    autoscaling.ScalingProcess.HEALTH_CHECK,
                    autoscaling.ScalingProcess.REPLACE_UNHEALTHY,
                    autoscaling.ScalingProcess.AZ_REBALANCE,
                    autoscaling.ScalingProcess.ALARM_NOTIFICATION,
                    autoscaling.ScalingProcess.SCHEDULED_ACTIONS
                ]
            )
        )

You can modify the update policy there ALL day long and the CDK will show NO diff. And, because NO diff is shown, it also won't deploy the changes either... It's only when you add a useless Output or some other resource ...

        core.CfnOutput(self, "force_asg_update_policy", value="true")

That the CDK diff will show a change (the output) and then you can deploy the CDK and it will actually update the stack. IF you DO NOT create/update an Output, or any other resource, the CDK DEPLOY will just say there's nothing to deploy and NOT deploy anything, even though the update_policy was modified... At least when you use a "rolling_update" policy, that is 100% the case.

*** I do believe that it is an actual CFN issue. I had the issue when I was writing YAML for CFN before I was introduced to the AWS CDK, which I by far prefer over CFN YAML. :-)

So, my point is... if Tags work that way, then it's not a one-off case, and just getting some kind of return codes like I mentioned could get us further in a direction of happiness for pipelines, while working to come up with solutions for these other random cases.

@timwelch
Copy link

timwelch commented Jan 6, 2021

I would be curious if there could be a use-case for pulling down the current CFN Template and diff it with the synthesized CFN? ... I can clearly see the update_policy / rolling_update changes in the cdk.out folder where it synthesizes what would be pushed. So there could be a comparison and even an error or warning that ... hey, though CDK diff says nothing changed... there's actually a discrepancy between the current / live CFN and what we would be pushing in place.

@cal5barton
Copy link

cal5barton commented Jan 25, 2021

@timwelch Running into a similar scenario with building our pipelines. I really like your suggestions.

@Doug-AWS @shivlaks does this issue need to be reopened or a separate issue logged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. package/tools Related to AWS CDK Tools or CLI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants