fix(ec2): export NatGatewayProvider for consistency with NatInstanceProvider

[crh23]

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]

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

1. Should I relabel it as a bug fix?
2. 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]

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]

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.

[msambol]

Few nits but looks good to me.

[TheRealAmazonKendra]

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.

[TheRealAmazonKendra]

What are these for?

[crh23]

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

[crh23]

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]

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]

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]

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]

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]

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]

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.