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

chore(guidelines): guidance on which constructs are included in the CDK #13218

Merged
merged 10 commits into from
Mar 2, 2021

Conversation

njlynch
Copy link
Contributor

@njlynch njlynch commented Feb 23, 2021

Adding a section of general guidance in the DESIGN_GUIDELINES to explain the L1,
L2, and beyond constructs, and what types of behaviors L2s should include versus
what types likely belong in separate repositories.

This guidance is by no means complete or without controversy. I expect pushback
and hope for both more supporting examples as well as counter examples.


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

Adding a section of general guidance in the DESIGN_GUIDELINES to explain the L1,
L2, and beyond constructs, and what types of behaviors L2s should include versus
what types likely belong in separate repositories.

This guidance is by no means complete or without controversy. I expect pushback
and hope for both more supporting examples as well as counter examples.
@gitpod-io
Copy link

gitpod-io bot commented Feb 23, 2021

@njlynch njlynch requested a review from a team February 23, 2021 14:27
@njlynch njlynch self-assigned this Feb 23, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Feb 23, 2021
@misterjoshua
Copy link
Contributor

misterjoshua commented Feb 24, 2021

Focusing-in on AppMesh, where's the line for a Virtual Gateway's compute? For instance, what about a construct that provides a load-balanced Fargate service for a Virtual Gateway: is it an included integration construct? included pattern? excluded? What about the similar, but for ECS with EC2-based capacity? Or how about EC2 instances in an ASG?

DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
@njlynch
Copy link
Contributor Author

njlynch commented Feb 24, 2021

Focusing-in on AppMesh, where's the line for a Virtual Gateway's compute? For instance, what about a construct that provides a load-balanced Fargate service for a Virtual Gateway: is it an included integration construct? included pattern? excluded? What about the similar, but for ECS with EC2-based capacity? Or how about EC2 instances in an ASG?

Disclaimer: I'll admit I'm not super familiar with AppMesh and its constructs.

Your example of a load-balanced Fargate service for a Virtual Gateway would be considered a pattern construct ("L3+") and (IMO) would not be a candidate for inclusion in the CDK. Generally, constructs that piece together multiple resources across multiple services to achieve a common use case are extremely useful, but also suffer from either exposing the full surface area of all of the underlying L2s or providing a subset of options that works for half of users but not the other half. We've found that publishing these as separate libraries and allowing users to customize and/or consume the options that fit their use cases is the better path forward.

@njlynch njlynch requested a review from eladb February 24, 2021 15:33
DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
@eladb eladb self-assigned this Feb 28, 2021
DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
@njlynch njlynch requested review from nija-at and eladb March 2, 2021 12:45
configure all resource properties, which requires a complete understanding of
the details of the underlying AWS CloudFormation resource model.

The next level of constructs, L2, also represent AWS resources, but with a
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth mentioning that L2 resources interact with other CloudFormation resources only via other L2s, never via L1s?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely worth capturing somewhere in this doc, but I'm not sure if this is the right section. Maybe under the Props section, or Implementation, depending on if we're talking about the public surface area or implementation details?

DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
DESIGN_GUIDELINES.md Show resolved Hide resolved
@njlynch njlynch requested a review from eladb March 2, 2021 15:08
DESIGN_GUIDELINES.md Outdated Show resolved Hide resolved
@eladb eladb changed the title chore: design guidance on construct abstraction and inclusion in the CDK feat(guidelines): guidance on which constructs are included in the CDK Mar 2, 2021
@njlynch njlynch changed the title feat(guidelines): guidance on which constructs are included in the CDK chore(guidelines): guidance on which constructs are included in the CDK Mar 2, 2021
@mergify
Copy link
Contributor

mergify bot commented Mar 2, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: d092144
  • Result: SUCCEEDED
  • 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 Mar 2, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit d884a34 into master Mar 2, 2021
@mergify mergify bot deleted the njlynch/l2-criteria branch March 2, 2021 16:49
cornerwings pushed a commit to cornerwings/aws-cdk that referenced this pull request Mar 8, 2021
…DK (aws#13218)

Adding a section of general guidance in the DESIGN_GUIDELINES to explain the L1,
L2, and beyond constructs, and what types of behaviors L2s should include versus
what types likely belong in separate repositories.

This guidance is by no means complete or without controversy. I expect pushback
and hope for both more supporting examples as well as counter examples.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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.

6 participants