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(apprunner): Add VPC Ingress Connection #22931

Closed
wants to merge 11 commits into from
Closed

feat(apprunner): Add VPC Ingress Connection #22931

wants to merge 11 commits into from

Conversation

JethroBakker-dotcom
Copy link

@JethroBakker-dotcom JethroBakker-dotcom commented Nov 16, 2022

Implements #22850

All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

@gitpod-io
Copy link

gitpod-io bot commented Nov 16, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team November 16, 2022 11:59
@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Nov 16, 2022
@corymhall
Copy link
Contributor

@JethroBakker-dotcom thanks for taking the time to work on this PR! Since this is a p2 and is a pretty big feature, we probably won't be able to review this in the short term.

@aws-cdk-automation
Copy link
Collaborator

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.

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! I have a few comments inline, but overall this looks very good!

const service = new apprunner.Service(this, 'Service', {
source: apprunner.Source.fromEcrPublic({
imageConfiguration: { port: 8000 },
imageIdentifier: 'public.ecr.aws/aws-containers/hello-app-runner:latest',
Copy link
Contributor

Choose a reason for hiding this comment

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

We should build this for the user instead of having them add the entire string, if possible. For instance, because of the function beingfromEcrPublic we already know that the prefix of it will be public.ecr.aws/ so this should only ask for the pieces that we don't know and build it for the user.

Choose a reason for hiding this comment

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

I understand your comment but I don't think it is related to this PR, other parts of the README also mention this type of imageIdentifier.

* Import from VPC Ingress Connection attributes.
*/
public static fromVpcIngressConnectionAttributes(scope: Construct, id: string, attrs: VpcIngressConnectionAttributes): IVpcIngressConnection {
const vpcIngressConnectionArn = attrs.vpcIngressConnectionArn;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a function to look up the VPC by ARN? fromVpcIngressConnectionArn

Copy link
Contributor

Choose a reason for hiding this comment

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

And one to look it up by name? fromVpcIngressConnectionName

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the BUILD FAILING 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.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review February 15, 2023 07:51

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 2afff1f
  • Result: FAILED
  • Build Logs (available for 30 days)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants