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: publish construct hierarchy with metadata to cloud assembly #4194

Merged
merged 13 commits into from
Sep 25, 2019

Conversation

nija-at
Copy link
Contributor

@nija-at nija-at commented Sep 22, 2019

Publish the CDK construct hierarchy with the construct id and path to
the cloud assembly directory, with a corresponding manifest entry.

Motivation and proposal documented in RFC - #4053.

This change focuses on integrating generating the metadata into the
cloud assembly as part of the the 'synthesize' lifecycle phase.
Subsequent changes will enrich the metadata file with additional
information from the construct tree (see RFC).

BREAKING CHANGES:

  • environment is no longer a property of CloudArtifact, instead is a property of its subclass - CloudformationArtifact.
  • construct.node.setContext() no longer works on the App construct. Use AppProps instead to pass context values.

The following CDK app generates the subsequent tree metadata JSON file

CDK app:

const app = new cdk.App();
const stack = new cdk.Stack(app, 'mystack');

const topic = new sns.Topic(this, 'mytopic');
const queue = new sqs.Queue(this, 'myqueue');
topic.addSubscription(new snssubs.SqsSubscription(queue));

app.synth();

Construct tree JSON:

{
  "version": "tree-0.1",
  "tree": {
    "id": "App",
    "path": "",
    "children": [
      {
        "id": "mystack",
        "path": "mystack",
        "children": [
          {
            "id": "mytopic",
            "path": "mystack/mytopic",
            "children": [
              {
                "id": "Resource",
                "path": "mystack/mytopic/Resource"
              }
            ]
          },
          {
            "id": "myqueue",
            "path": "mystack/myqueue",
            "children": [
              {
                "id": "Resource",
                "path": "mystack/myqueue/Resource"
              },
              {
                "id": "Policy",
                "path": "mystack/myqueue/Policy",
                "children": [
                  {
                    "id": "Resource",
                    "path": "mystack/myqueue/Policy/Resource"
                  }
                ]
              },
              {
                "id": "mystackmytopic28FA060E",
                "path": "mystack/myqueue/mystackmytopic28FA060E",
                "children": [
                  {
                    "id": "Resource",
                    "path": "mystack/myqueue/mystackmytopic28FA060E/Resource"
                  }
                ]
              }
            ]
          }
        ]
      },
      {
        "id": "ConstructTreeMetadata",
        "path": "ConstructTreeMetadata"
      }
    ]
  }
}

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

Publish the CDK construct hierarchy with the construct id and path to
the cloud assembly directory, with a corresponding manifest entry.

Motivation and proposal documented in RFC - #4053.

This change focuses on integrating generating the metadata into the
cloud assembly as part of the the 'synthesize' lifecycle phase.
Subsequent changes will enrich the metadata file with additional
information from the construct tree (see RFC).
@nija-at nija-at self-assigned this Sep 22, 2019
@mergify
Copy link
Contributor

mergify bot commented Sep 22, 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: FAILED
  • Build Logs (available for 30 days)

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

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Add an example output in the PR description

@nija-at
Copy link
Contributor Author

nija-at commented Sep 22, 2019

Note, the inclusion of a BREAKING CHANGE in the PR description.

@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

@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

* Rename ConstructTreeMetadata to Tree
* Wrap tree construct with a version number
* Move Tree construct into the constructor
@nija-at nija-at added the pr/do-not-merge This PR should not be merged at this time. label Sep 23, 2019
@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

@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 requested a review from eladb September 24, 2019 08:08
@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: SUCCEEDED
  • Build Logs (available for 30 days)

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


/**
* Include construct tree metadata as part of the Cloud Assembly.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a "@see" with a link to the RFC

Copy link
Contributor Author

@nija-at nija-at Sep 24, 2019

Choose a reason for hiding this comment

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

Sure. I'll wait until the RFC is published so I can get the a more stable link.

Niranjan Jayakar and others added 2 commits September 24, 2019 15:03
* Renamed Tree to TreeMetadata
* Re-typed 'metadata' to 'tree' in CloudArtifacts
@nija-at nija-at requested a review from eladb September 24, 2019 14:04
@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: 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: SUCCEEDED
  • Build Logs (available for 30 days)

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

@nija-at nija-at merged commit 3cca03d into master Sep 25, 2019
@nija-at nija-at deleted the nija-at/app-metadata branch September 25, 2019 08:29
@@ -80,7 +84,8 @@ export class CloudArtifact {
switch (artifact.type) {
case ArtifactType.AWS_CLOUDFORMATION_STACK:
return new CloudFormationStackArtifact(assembly, id, artifact);

case ArtifactType.CDK_TREE:
return new TreeCloudArtifact(assembly, id, artifact);
default:
throw new Error(`unsupported artifact type: ${artifact.type}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

IMPORTANT: Just realized that this change requires a bump in the cx-protocol version because we were stupid enough to throw an error when reading a cloud assembly with unrecognized artifact types (no future compatibility). This means that an old CLI will crash if it will try to synth a CDK app that has this change.

Please also remove this default: section and simply ignore unknown artifacts. That should be the correct behavior going forward so we can avoid such issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

At least make it a warning or something right. Not srue that ignoring is the right behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

The warning should say something like "Unrecognized artifact type: ${type}. You need to upgrade your CDK CLI to use this artifact" or something. Give users a hint on how to remedy this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it makes total sense to issue a warning from this library. Maybe the CLI can issue a warning if this returns undefined

nija-at pushed a commit that referenced this pull request Sep 26, 2019
Since we've added a new type of artifact to the cloud assembly, namely
CDK_TREE, the protocol version needs to be bumped. This will result in
CDK customers being notified to upgrade the CDK CLI to >= 1.10.0.

To prevent this from occurring in the future, the change also changes
the behaviour when an unknown artifact type is detected when reading the
cloud assembly manifest. This ensures that the CLI will be forwards
compatible to cloud assemblies when new artifact types are added to
them.

See #4194 (comment)
mergify bot pushed a commit that referenced this pull request Sep 26, 2019
Since we've added a new type of artifact to the cloud assembly, namely
CDK_TREE, the protocol version needs to be bumped. This will result in
CDK customers being notified to upgrade the CDK CLI to >= 1.10.0.

To prevent this from occurring in the future, the change also changes
the behaviour when an unknown artifact type is detected when reading the
cloud assembly manifest. This ensures that the CLI will be forwards
compatible to cloud assemblies when new artifact types are added to
them.

See #4194 (comment)
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
alukach pushed a commit to alukach/aws-cdk that referenced this pull request Nov 1, 2019
Since we've added a new type of artifact to the cloud assembly, namely
CDK_TREE, the protocol version needs to be bumped. This will result in
CDK customers being notified to upgrade the CDK CLI to >= 1.10.0.

To prevent this from occurring in the future, the change also changes
the behaviour when an unknown artifact type is detected when reading the
cloud assembly manifest. This ensures that the CLI will be forwards
compatible to cloud assemblies when new artifact types are added to
them.

See aws#4194 (comment)
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. pr/do-not-merge This PR should not be merged at this time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants