Skip to content

Conversation

@rzamana
Copy link
Contributor

@rzamana rzamana commented Dec 22, 2020

closes #276

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Dec 22, 2020

@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label Dec 22, 2020
rix0rrr
rix0rrr previously requested changes Jan 6, 2021
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

I like it, thanks!

With one change: I think we don't need the feature flag, this feels like something that can be enabled wholesale. The limit is unlikely to change anytime soon (it recently changed), and it's unlikely that there are valid use cases for having >500 resources in an undeployable template.

If you feel uncomfortable doing this, then I'd suggest putting the resource limit in a context settings, so people could set something like:

"context": {
  "@aws-cdk/core.stackResourceLimit": 1000,
}

If they feel like it (and potentially we could have have 0 mean "no limit" to switch it off).

But honestly I don't think there's a need for such an elaborate mechanism.

if (numberOfResources > this.maxResources) {
throw new Error(`Number of resources: ${numberOfResources} is greater than allowed maximum of ${this.maxResources}`);
} else if (numberOfResources >= (this.maxResources * 0.8)) {
Annotations.of(this).addWarning(`Number of resources: ${numberOfResources} is approaching allowed maximum of ${this.maxResources}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change this into addMessage.

A warning may have the effect of breaking deployment, if deployment is run using --strict (which turns warnings into errors).

This is a warning, but it's a different kind of warning from the regular ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you mean addInfo?

Because the addMessage is a private method from Annotations!

Copy link
Contributor

@rix0rrr rix0rrr Jan 11, 2021

Choose a reason for hiding this comment

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

Oh, doh. Yes addInfo.

@rzamana rzamana changed the title feat(core): Adding maximum resources validation to the stack (under feature flag) feat(core): Adding maximum resources validation to the stack Jan 8, 2021
@mergify mergify bot dismissed rix0rrr’s stale review January 8, 2021 10:43

Pull request has been modified.

@rix0rrr rix0rrr changed the title feat(core): Adding maximum resources validation to the stack feat(core): validate maximum amount of resources in a stack Jan 11, 2021
rix0rrr
rix0rrr previously approved these changes Jan 11, 2021
@mergify mergify bot dismissed rix0rrr’s stale review January 11, 2021 13:32

Pull request has been modified.

rix0rrr
rix0rrr previously approved these changes Jan 11, 2021
@mergify mergify bot dismissed rix0rrr’s stale review January 11, 2021 14:28

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Jan 11, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 771d283
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Jan 11, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 26121c8 into aws:master Jan 11, 2021
@rzamana rzamana deleted the feature/resource_limit branch January 11, 2021 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

@aws-cdk/core Related to core CDK functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Warn/fail synthesis when reached the 200 resource limit

3 participants