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

feat(toolkit): improve diff user interface #1187

Merged
merged 6 commits into from
Nov 18, 2018
Merged

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Nov 15, 2018

To display construct path we fuse metadata from the synthesized output
(CDK metadata) and info from the the "aws:cdk:path" CloudFormation
metadata (if exists).

screen shot 2018-11-15 at 11 19 30 pm

TODO:

  • Run toolkit integration tests
  • Perform some manual tests (eg run the CDK workshop)
  • Rebase and change target to “master”

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

Elad Ben-Israel added 2 commits November 15, 2018 23:10
`DifferenceCollection#applyFilter` can be used to delete any changes
that do not pass the predicate from the collection.

Changed `DifferenceCollection#count` to do a lazy calculation since now
`changes` is mutable.

The toolkit switch `cdk diff --strict` disables this behavior.

Fixes #465
- When possible, display element's construct path alongside 
  logical ID (fixes #1121)
- Sort changes according to type: removed > added > updated > other
- Add section headers: parameters, resources, 
  output (fixes #1120)
- Reduce clutter and emojis
 
To display construct path we fuse metadata from the synthesized output
(CDK metadata) and info from the the "aws:cdk:path" CloudFormation
metadata (if exists).
@eladb eladb requested review from RomainMuller, rix0rrr and a team November 15, 2018 21:20
@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 16, 2018

If we resolve { Ref }s to parameters while doing this operation, we'll have solved #395 as well.

Copy link
Contributor

@RomainMuller RomainMuller left a comment

Choose a reason for hiding this comment

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

I am profoundly shocked you put clutter and emojis in the same bulk.

@eladb eladb changed the base branch from benisrae/hide-metadata-resource to master November 18, 2018 12:12
@eladb eladb merged commit 9c3c5c7 into master Nov 18, 2018
@eladb eladb deleted the benisrae/diff-ui-improv branch November 18, 2018 13:25
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 23, 2019
@mergify
Copy link
Contributor

mergify bot commented Sep 23, 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
4 participants