Skip to content

Conversation

@nija-at
Copy link
Contributor

@nija-at nija-at commented Oct 17, 2019

Also, switch the children node from an array to an object with each child object keyed on its id.

TODO:

  • Support for tokens

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

…ructs to tree.json

Also, switch the children node from an array to an object with each
child object keyed on its id.
@nija-at nija-at added the pr/do-not-merge This PR should not be merged at this time. label Oct 17, 2019
@nija-at nija-at requested review from a team and shivlaks October 17, 2019 13:45
@nija-at nija-at requested a review from eladb as a code owner October 17, 2019 13:45
@nija-at nija-at self-assigned this Oct 17, 2019
@mergify
Copy link
Contributor

mergify bot commented Oct 17, 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

@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

@SomayaB SomayaB added the contribution/core This is a PR that came from AWS. label Oct 17, 2019
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.

This seems upside down...

I don't think CfnResources should be special nor that the tree should encode the concept of type and properties. I believe the design we discussed was that each tree node has a set of key-value attributes and constructs can "contribute" to this attribute bag by implementing some interface. Then CfnResource implements the interface and contributes the attributes aws.cloudformation.type and aws.cloudformation.properties (which will be key/value pairs).

Something like:

interface ITreeAttributes {
  treeAttributes: { [key: string]: any };
}

And then:

class CfnResource implements ITreeAttributes {
  public get treeAttributes() {
    return {
      'aws:cloudformation:type': this.resourceType,
      'aws:cloudformation:properties': this.cfnProperties
    }
  }
}

@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 Oct 18, 2019
@shivlaks
Copy link
Contributor

This seems upside down...

I don't think CfnResources should be special nor that the tree should encode the concept of type and properties. I believe the design we discussed was that each tree node has a set of key-value attributes and constructs can "contribute" to this attribute bag by implementing some interface. Then CfnResource implements the interface and contributes the attributes aws.cloudformation.type and aws.cloudformation.properties (which will be key/value pairs).

Something like:

interface ITreeAttributes {
  treeAttributes: { [key: string]: any };
}

And then:

class CfnResource implements ITreeAttributes {
  public get treeAttributes() {
    return {
      'aws:cloudformation:type': this.resourceType,
      'aws:cloudformation:properties': this.cfnProperties
    }
  }
}

This is my miss for not closing the RFC sooner. Yes we did decide that we would have an attribute bag and constructs will provide information into it. L1's would report back all the properties in toCloudFormation. L2s would optionally contribute properties that are configurable, properties of interest, etc.

Type was the thing we hadn't closed discussion on. Initially, I felt it's nice to have as a top-level attribute of a Node, but I agree that since it's not applicable to every construct, that it's better off living in the attribute bag.

I'll update the RFC to make the change to nest type into the attribute bag of a tree node, rather than a top-level attribute.

@nija-at nija-at closed this Oct 22, 2019
shivlaks added a commit that referenced this pull request Nov 11, 2019
…tructs to tree.json (#4894)

Modifies the children node from an array to an object with each child object keyed on its id. Also added an interface `IInspectable` that constructs can optionally implement to contribute attributes into `tree.json`.

Generated classes for Cfn resources implement `IInspectable` and contribute their resource type and props in the attribute bag.

Supercedes #4562
@nija-at nija-at deleted the nija-at/treemd-l1type branch November 22, 2019 10:30
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