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

rfc: tree.json #4053

Merged
merged 10 commits into from
Jan 23, 2020
Merged

rfc: tree.json #4053

merged 10 commits into from
Jan 23, 2020

Conversation

shivlaks
Copy link
Contributor

Strategy for publishing the construct tree which represents a CDK app as a part of the Cloud Assembly that the CDK generates.


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

@shivlaks shivlaks added the pr/do-not-merge This PR should not be merged at this time. label Sep 13, 2019
@shivlaks shivlaks requested review from a team September 13, 2019 00:24
@mergify
Copy link
Contributor

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

@NGL321 NGL321 assigned ghost Sep 13, 2019
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

This looks solid. Clear and concise and describes the benefits well.

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.

See comments

3. ***Metadata -*** Constructs will expose metadata which will include an array of objects associated with the construct.
1. **Location** - The full path of the location where the construct is defined. This will support use cases where the construct-tree nodes can support navigability back to the declaration in the IDE itself.
2. ***Properties*** - Constructs will opt into the set of properties, and these will be exposed. Higher level constructs (L2 and L3) will expose properties and values of the resources they contain and not necessarily enumerate every property that’s available for a resource. This will especially be useful for constructs that are opinionated and set defaults on behalf of developers. Low level constructs (L1) will expose all properties that are configured.
3. ***Errors/Warnings*** - Errors and warnings that are produced during synthesis indicating validation failures, deprecation notices, guidance, etc will be included in the tree. These are emitted at the construct level although the message may point towards a specific property. The construct tree should be complete and include validation failures for all resources in the CDK application rather than just just fail on the first error.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do errors & warnings fall under "minimal set of requirements"?

2. **Constructs**
1. ***Nodes*** - CDK application constructs at the root and drill down to the constructs it contains (1+ cloud resources.) Initially, we will focus our attention on AWS resources, but the model is extensible to cloud components from any provider
2. **Types/Hierarchy** - Types and hierarchy information will contain the details around the level of abstraction (L1, L2, L3..), service(s), and resource(s) that they represent.
3. ***Metadata -*** Constructs will expose metadata which will include an array of objects associated with the construct.
Copy link
Contributor

Choose a reason for hiding this comment

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

The 'Metadata' group sounds vague and it's not clear why these 3 fall under this but not 'Type'.

I'd suggest dropping this grouping in this requirements section altogether. Perhaps something along the lines of -
"The following information will be exposed for all instances of all CDK constructs, involved in the application. They are (a) Type & hierarchy - ... (b) source location - .... (c) ..."

Would that be better?

1. **Format** - Publish underlying data as a `.json` file that contains the information required to render a construct tree view. The construct tree illustrated in the previous section is only a depiction of what a human readable(ish) view might look like
2. **Constructs**
1. ***Nodes*** - CDK application constructs at the root and drill down to the constructs it contains (1+ cloud resources.) Initially, we will focus our attention on AWS resources, but the model is extensible to cloud components from any provider
2. **Types/Hierarchy** - Types and hierarchy information will contain the details around the level of abstraction (L1, L2, L3..), service(s), and resource(s) that they represent.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I follow this.

  1. Type - Is this the Type of the construct within CDK, or the type of resource (in the case of an L1) this renders into. Which one do we want? I suppose both.

  2. What's hierarchy? And what exactly do we want to expose? From what I understand, there are two things here - one is the hierarchy of CDK Construct classes (viz. SingletonLambda → FunctionBase → IFunction) and the other is the hierarchy of how a specific (AWS) Resource got created. They're both valuable to power different experiences.

|--- |--- |--- |--- |
|sourceLocation |string |Required |location in source code where the construct is defined |
|resourceType |string |Required |type of resource (schema / classification terminology TBD) |
|description |string |Not Required |description of the construct |
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 and why is it needed?

nija-at pushed a commit that referenced this pull request 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).
nija-at added a commit that referenced this pull request Sep 25, 2019
Publish the CDK construct tree 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 output with additional
information about each construct (see RFC).
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 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: 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 Dec 16, 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

@mergify
Copy link
Contributor

mergify bot commented Dec 16, 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

@eladb eladb added the management/rfc request for comments label Dec 16, 2019
@mergify
Copy link
Contributor

mergify bot commented Dec 16, 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

@eladb
Copy link
Contributor

eladb commented Dec 16, 2019

@shivlaks @nija-at what's up here? Is this ready for merge?

@mergify
Copy link
Contributor

mergify bot commented Dec 16, 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

2 similar comments
@mergify
Copy link
Contributor

mergify bot commented Dec 16, 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

@mergify
Copy link
Contributor

mergify bot commented Dec 16, 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

@shivlaks shivlaks requested review from nija-at and eladb January 22, 2020 18:43
@shivlaks
Copy link
Contributor Author

@shivlaks @nija-at what's up here? Is this ready for merge?

@eladb @nija-at can I get another review on this, is there anything I've missed?

@shivlaks shivlaks assigned eladb and unassigned ghost Jan 22, 2020
@mergify
Copy link
Contributor

mergify bot commented Jan 22, 2020

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@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 eladb changed the title rfc: publish cdk construct tree rfc: tree.json Jan 22, 2020
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.

looks good go ahead.

please move it to the RFC repo after merge and create a tracking issue following the RFC workflow.

@shivlaks shivlaks merged commit 2ab25bd into master Jan 23, 2020
@shivlaks shivlaks deleted the shivlaks/rfc-construct-tree branch January 23, 2020 20:19
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. management/rfc request for comments 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