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

chore(core): resolve tokens before publishing tree.json #4984

Merged
merged 6 commits into from
Nov 13, 2019
Merged

chore(core): resolve tokens before publishing tree.json #4984

merged 6 commits into from
Nov 13, 2019

Conversation

nija-at
Copy link
Contributor

@nija-at nija-at commented Nov 12, 2019 •
edited
Loading

Resolve tokens before publishing tree.json

Add a try-catch block around the construct tree metadata generation to
prevent failure of synthesis. Construct tree metadata is not a critical
step.

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

@nija-at nija-at added the contribution/core This is a PR that came from AWS. label Nov 12, 2019
@nija-at nija-at requested review from eladb, shivlaks and a team November 12, 2019 16:01
@nija-at nija-at self-assigned this Nov 12, 2019
@mergify
Copy link
Contributor

mergify bot commented Nov 12, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@nija-at nija-at removed their assignment Nov 13, 2019
eladb
eladb reviewed Nov 13, 2019
View reviewed changes
packages/@aws-cdk/core/lib/private/tree-metadata.ts Outdated
});
} catch (e) {
// Prevent errors here to fail synthesis, until this module stays @experimental.
this.node.addWarning(`tree.json could not be generated - ${e}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

This means that if any visit fails, the entire tree will not be synthed, which seems harsh. Wouldn't it make more sense to only surround the user code (call to visit) with this all-encompassing catch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would rather provide no tree.json (which the consumer, an IDE for instance, can interpret as an error) rather than one that has missing information (and no indication that it is missing information).

Copy link
Contributor

Choose a reason for hiding this comment

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

because of the extensible nature of this, I don't think it makes sense to fail the entire thing. Say you imported some 3rd party construct library that throw an exception in inspect. Suddenly, you can't see anything.

Maybe we can synthesize an "error" node or something if a node fails "visit", but I'd argue that failing the entire tree is not the right granularity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. We could even collect and throw all the errors into the cloud manifest under the 'tree' metadata, which the consumer can collect and display.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think .addWarning should do the trick

packages/@aws-cdk/core/lib/private/tree-metadata.ts Outdated Show resolved Hide resolved
@nija-at nija-at requested a review from eladb November 13, 2019 11:27
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • 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 Nov 13, 2019

Thank you for contributing! Your pull request is now being automatically merged.

@mergify mergify bot merged commit cf54c51 into master Nov 13, 2019
@mergify mergify bot deleted the nija-at/core-treemd-tokens branch November 13, 2019 17:29
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

eladb pushed a commit that referenced this pull request Nov 13, 2019
Revert "chore(core): resolve tokens before publishing tree.json (#4984)"
3c1ede1 
This reverts commit cf54c51.
arhea pushed a commit to arhea/aws-cdk that referenced this pull request Nov 14, 2019
Merge branch '4295-windows-ecs-support' of github.com:arhea/aws-cdk i…
6258122 
…nto 4295-windows-ecs-support

* '4295-windows-ecs-support' of github.com:arhea/aws-cdk:
  chore(deps-dev): bump @types/lodash from 4.14.146 to 4.14.147 (aws#5021)
  Revert "fix(assets): support exceptions to exclude patterns (aws#4473)" (aws#5022)
  chore(deps): bump jsii-pacmak from 0.20.3 to 0.20.5 (aws#5003)
  chore(deps): bump codemaker from 0.20.3 to 0.20.5 (aws#5007)
  chore(deps-dev): bump @types/jest from 24.0.22 to 24.0.23 (aws#4993)
  chore(deps): bump jsii from 0.20.3 to 0.20.5 (aws#5006)
  chore(deps-dev): bump jsii-diff from 0.20.3 to 0.20.5 (aws#5005)
  chore(deps): bump jsii-spec from 0.20.3 to 0.20.5 (aws#5008)
  chore(core): resolve tokens before publishing tree.json (aws#4984)
  feat(cli): adding new option to `cdk deploy` to indicate whether ChangeSet should be executed (aws#4852)
  chore: move semantic.yaml to .github/
  fix(core): unable to find stack by name using the cli in legacy mode (aws#4998)
  fix(ecs-patterns): Fix issue related to protocol being passed to target group (aws#4988)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eladb eladb eladb approved these changes

@shivlaks shivlaks Awaiting requested review from shivlaks

Assignees
No one assigned
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nija-at @aws-cdk-automation @eladb