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: add an example construct package #7748

Merged
merged 1 commit into from May 7, 2020

Conversation

skinny85
Copy link
Contributor

@skinny85 skinny85 commented May 2, 2020

Commit Message

feat: add an example construct package

This change adds a new package whose sole purpose is to illustrate the many
concepts and patterns used in the CDK Construct Library.
The idea is it would serve as the jumping-off point for contributions to the main CDK repo,
and also creating independent construct libraries.

End Commit Message


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

@skinny85 skinny85 requested a review from rix0rrr May 2, 2020 00:38
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label May 2, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 8b96a45
  • Result: FAILED
  • Build Logs (available for 30 days)

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

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

In general I love this, but to me it looks like this example is trying to be 2 things at the same time.

It's trying to be a superset of explanation of anything you might want to stuff into a construct, as well as a copy/pasteable base.

Consequence of trying to be the 2nd while also being the 1st, is that there is a LOT OF STUFF in the template that will have to be deleted by a knowledgeable user, and it might not be obvious which parts are optional and which parts aren't.

I think a simple or more additive approach might work better...

I don't have a good ready-made solution either way, except maybe looking into some templating tools like https://github.com/cookiecutter/cookiecutter or https://www.npmjs.com/package/cookiecutter or similar.

I understand you don't really want to go for those, and I will ship this to not let perfect be the enemy of good, but maybe we can spend a couple more hours of thinking about it?

packages/@aws-cdk/example-construct-library/package.json Outdated Show resolved Hide resolved
packages/@aws-cdk/example-construct-library/package.json Outdated Show resolved Hide resolved
packages/@aws-cdk/example-construct-library/package.json Outdated Show resolved Hide resolved
import { exampleResourceArnComponents } from './example-resource-utils';

/**
* The interface that represents the ExampleResource resource.
Copy link
Contributor

Choose a reason for hiding this comment

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

Fucking AMAZING job on all the explanations... but I wonder if this is the best place for them.

My experience with explanatory-comments-in-templates is that after a while people just start skipping them and leaving them in (instead of deleting them). So my gut says this all shouldn't live here... unfortunately I don't have a better solution.

Best I can come up with is put all the explanation (basically the Construct Library Guide) up on the GitHub wiki, and putting links to the relevant sections into the comment blocks...

?

@rix0rrr
Copy link
Contributor

rix0rrr commented May 4, 2020

Also wondering if we should separate out the ResourceBase and the Resource into separate files? Will become very useful if they start growing large, better get an early jump on it?

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: aa29b7f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@skinny85 skinny85 force-pushed the feat/example-construct-library branch from aa29b7f to 854138b Compare May 7, 2020 01:23
@skinny85 skinny85 requested review from rix0rrr and nija-at May 7, 2020 01:24
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 854138b
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@rix0rrr rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label May 7, 2020
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

I think we've beaten this horse enough. Let's get this in and tweak it further as we see the need to.

Blocked by a label if there's any more changes you'd like to get in, otherwise let's do this.

@skinny85 skinny85 removed the pr/do-not-merge This PR should not be merged at this time. label May 7, 2020
This change adds a new package whose sole purpose is to illustrate the many
concepts and patterns used in the CDK Construct Library.
The idea is it would serve as the jumping-off point for contributions to the main CDK repo,
and also creating independent construct libraries.
@skinny85 skinny85 force-pushed the feat/example-construct-library branch from 854138b to 2ef1e3e Compare May 7, 2020 22:56
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 2ef1e3e
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@skinny85 skinny85 merged commit 2223584 into aws:master May 7, 2020
@skinny85 skinny85 deleted the feat/example-construct-library branch May 7, 2020 23:41
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
Development

Successfully merging this pull request may close these issues.

None yet

5 participants