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

docs: add clarity for creating own VPC resource provider #716

Merged
merged 5 commits into from
Jun 12, 2023

Conversation

zjaco13
Copy link
Contributor

@zjaco13 zjaco13 commented Jun 6, 2023

Issue #, if available: #632

Description of changes:
Added documentation to explain creating VPC Resource Providers and added links to other resource providers

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Collaborator

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

@zjaco13 Thanks for the PR, have few minor comments.

@@ -40,7 +40,22 @@ Example implementations:
class VpcResourceProvider implements ResourceProvider<IVpc> {
provide(context: ResourceContext): IVpc {
const scope = context.scope; // stack
...
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rather add this is as a separate example that over writing on existing one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@@ -14,7 +14,7 @@ import * as blueprints from '@aws-quickstart/eks-blueprints';

const app = new cdk.App();

const addOn = new blueprints.addons.ClusterAutoscalerAddOn();
const addOn = new blueprints.addons.ClusterAutoScalerAddOn();
Copy link
Collaborator

Choose a reason for hiding this comment

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

What changed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed the s in ClusterAutoscalerAddOn to uppercase, since that's how you have to call the addon

Copy link
Collaborator

@shapirov103 shapirov103 left a comment

Choose a reason for hiding this comment

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

@zjaco13 please see my comment and let me know if you have any questions. @elamaran11 please take a look at my comment as well.

docs/resource-providers/index.md Show resolved Hide resolved
Copy link
Collaborator

@shapirov103 shapirov103 left a comment

Choose a reason for hiding this comment

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

LGTM. @elamaran11 please add review and merge if all good.

Copy link
Collaborator

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

LGTM

@elamaran11
Copy link
Collaborator

/do-e2e-tests

@elamaran11 elamaran11 merged commit 6df9add into aws-quickstart:main Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants