fix(ec2): export NatGatewayProvider for consistency with NatInstanceProvider by crh23 · Pull Request #28810 · aws/aws-cdk

crh23 · 2024-01-22T13:11:28Z

Export NatGatewayProvider for two reasons: to allow instantiation with new, and to make docs present it as an implementation of NatProvider.
Also added a unit test for the same "functionality" for NatInstanceProvider, for symmetry with the added test for NatGatewayProvider.
Unit test and integration test for new functionality added.

Closes #28372.

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

crh23 · 2024-01-22T13:27:32Z

Clarification Request: this change is somewhere between a feature and a bug fix (actual change is tiny).

Should I relabel it as a bug fix?
If not, is a README change required? The enabled functionality is very minor, and the previously-available version isn't in the README currently (closest is "Using NAT instances")

crh23 · 2024-01-30T16:45:44Z

Applying a bit more thought, I think this is a bug fix more than a feature - the change to actual functionality is the addition of a single export, and it seems unintentional that that wasn't already present. Labels will probably be all messed up, sorry

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

aws-cdk-automation · 2024-03-05T00:07:57Z

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

# Conflicts: # packages/aws-cdk-lib/aws-ec2/test/vpc.test.ts

packages/aws-cdk-lib/aws-ec2/test/vpc.test.ts

msambol

Few nits but looks good to me.

TheRealAmazonKendra · 2024-04-16T23:32:18Z

Hmm, not totally sure how I feel about this one, not because anything you've done here is invalid but because it looks to me like the error here was exporting the other function, not the lack of export on this one. We can't undo the export because that would be a breaking change, but I'm not sure if it's better to have it consistently wrong in both places or inconsistent.

packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.nat-gateway-provider.ts

TheRealAmazonKendra · 2024-04-16T23:24:24Z

packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.nat-gateway-provider.ts

+    Array.isArray(vpc);
+    Array.isArray(natGatewayProvider.configuredGateways);


What are these for?

Based the test on https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.nat-instances.lit.ts, which includes the assertions

…t, rename unit tests

crh23 · 2024-04-17T11:06:23Z

Hmm, not totally sure how I feel about this one, not because anything you've done here is invalid but because it looks to me like the error here was exporting the other function, not the lack of export on this one. We can't undo the export because that would be a breaking change, but I'm not sure if it's better to have it consistently wrong in both places or inconsistent.

I've seen folks be confused by the current inconsistency. In the design of the CDK docs, the natural way (for me) to find the implementations of an abstract class (such as NatProvider) is to view the classes listed in Implemented by. Currently this misses NatGatewayProvider, but includes NatInstanceProvider. The new NatInstanceProviderV2, which was added since I raised this CR, is also exported.

I think I agree that it would be best to export none of them (and add a usage note to the docs of NatProvider), but that's not an option now, and I think exporting one but not the other (current situation) is worse than exporting all (this CR).

TheRealAmazonKendra

I've seen folks be confused by the current inconsistency. In the design of the CDK docs, the natural way (for me) to find the implementations of an abstract class (such as NatProvider) is to view the classes listed in Implemented by. Currently this misses NatGatewayProvider, but includes NatInstanceProvider. The new NatInstanceProviderV2, which was added since I raised this CR, is also exported.

I think I agree that it would be best to export none of them (and add a usage note to the docs of NatProvider), but that's not an option now, and I think exporting one but not the other (current situation) is worse than exporting all (this CR).

Fair point.

YOLO

Pull request has been modified.

TheRealAmazonKendra · 2024-07-31T17:50:15Z

Drat. This failing test has nothing to do with your change and everything to do with how long your waiting on this approval. I'll fix it.

crh23 · 2024-07-31T18:35:56Z

Drat. This failing test has nothing to do with your change and everything to do with how long your waiting on this approval. I'll fix it.

Ack - I recall getting integ tests working in my environment was a bit of a chore, so happy to let you do it this time 😁
(assuming that is what the issue is)

Pull request has been modified.

TheRealAmazonKendra

YOLO Again. I should probably stop saying that in official PR reviews... ¯\(ツ)/¯

mergify · 2024-07-31T19:23:24Z

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

aws-cdk-automation · 2024-07-31T20:03:30Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 08579ca
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2024-07-31T20:03:56Z

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

github-actions · 2024-07-31T20:04:11Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

Chris Hughes and others added 8 commits December 15, 2023 18:32

export NatGatewayProvider for consistency with NatInstanceProvider

f2be2e0

Merge branch 'aws:main' into nat-gateway-export

282447f

Merge branch 'aws:main' into nat-gateway-export

0e6fabf

Lint

7dd8d80

Attempt to add working integration test

8dbf12e

rename integration test to new style

fdd591b

Clean up

2799af8

Merge branch 'aws:main' into nat-gateway-export

14b2a77

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Jan 22, 2024

aws-cdk-automation requested a review from a team January 22, 2024 13:11

aws-cdk-automation previously requested changes Jan 22, 2024

View reviewed changes

crh23 changed the title ~~feat(aws-ec2) export NatGatewayProvider for consistency with NatInstanceProvider~~ feat(aws-ec2): export NatGatewayProvider for consistency with NatInstanceProvider Jan 22, 2024

aws-cdk-automation added the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Jan 22, 2024

crh23 changed the title ~~feat(aws-ec2): export NatGatewayProvider for consistency with NatInstanceProvider~~ feat(ec2): export NatGatewayProvider for consistency with NatInstanceProvider Jan 22, 2024

crh23 marked this pull request as ready for review January 22, 2024 13:39

Merge branch 'main' into nat-gateway-export

b4e77b6

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 26, 2024

crh23 changed the title ~~feat(ec2): export NatGatewayProvider for consistency with NatInstanceProvider~~ fix(ec2): export NatGatewayProvider for consistency with NatInstanceProvider Jan 30, 2024

github-actions bot mentioned this pull request Feb 1, 2024

Monthly PR metrics report - Jan 2024 #28952

Closed

Merge branch 'main' into nat-gateway-export

adc54b0

# Conflicts: # packages/aws-cdk-lib/aws-ec2/test/vpc.test.ts

msambol reviewed Mar 21, 2024

View reviewed changes

packages/aws-cdk-lib/aws-ec2/test/vpc.test.ts Outdated Show resolved Hide resolved

msambol reviewed Mar 21, 2024

View reviewed changes

packages/aws-cdk-lib/aws-ec2/test/vpc.test.ts Outdated Show resolved Hide resolved

msambol suggested changes Mar 21, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Mar 21, 2024

TheRealAmazonKendra reviewed Apr 16, 2024

View reviewed changes

crh23 added 2 commits April 17, 2024 11:39

Merge branch 'refs/heads/main' into nat-gateway-export

c1533fd

Remove pragma markup, remove redundant assertion of vpc in NAG GW tes…

661bed1

…t, rename unit tests

TheRealAmazonKendra previously approved these changes Jul 31, 2024

View reviewed changes

Merge branch 'main' into nat-gateway-export

fa0c7ae

TheRealAmazonKendra previously approved these changes Jul 31, 2024

View reviewed changes

update snapshots

bd8294c

TheRealAmazonKendra approved these changes Jul 31, 2024

View reviewed changes

Merge branch 'main' into nat-gateway-export

08579ca

mergify bot merged commit fbc28bc into aws:main Jul 31, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2024

		Array.isArray(vpc);
		Array.isArray(natGatewayProvider.configuredGateways);

Conversation

crh23 commented Jan 22, 2024

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

crh23 commented Jan 22, 2024

Uh oh!

crh23 commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aws-cdk-automation commented Mar 5, 2024

Uh oh!

Uh oh!

Uh oh!

msambol left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra commented Apr 16, 2024

Uh oh!

Uh oh!

TheRealAmazonKendra Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

crh23 Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

crh23 commented Apr 17, 2024

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra commented Jul 31, 2024

Uh oh!

crh23 commented Jul 31, 2024

Uh oh!

TheRealAmazonKendra left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jul 31, 2024

Uh oh!

aws-cdk-automation commented Jul 31, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jul 31, 2024

Uh oh!

github-actions bot commented Jul 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

crh23 commented Jan 30, 2024 •

edited

Loading

TheRealAmazonKendra left a comment •

edited

Loading