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

fix(servicecatalog): Make possible to have assets in different products in the same portfolio #26039

Closed
wants to merge 6 commits into from

Conversation

ViktarKhomich
Copy link

fix(servicecatalog): Make possible to have assets in different products in the same portfolio (#25189)

Currently it's not possible to have assets (ex. lambdas) in different products in the same portfolio. The reason for that is that asset bucket will be deployed
within the portfolio stack and name for its deployment is hardcoded what leads to the exception 'There is already a Construct with name' at synth step.

Closes #25189

Exemption Request


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 Jun 19, 2023

@github-actions github-actions bot added bug This issue is a bug. effort/large Large work item – several weeks of effort p1 labels Jun 19, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team June 19, 2023 14:24
@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Jun 19, 2023
@ViktarKhomich
Copy link
Author

Hi Aws Cdk Team! Could you please run integration test for me?
yarn integ test/aws-servicecatalog/test/integ.product.js --no-clean --update-on-failed

When I am doing it on my own, I am getting this error (region seems to be set in env variables):

❌ Building assets failed: Error: Building Assets Failed: Error: Could not assume role in target account using current credentials (which are for account ....) Inaccessible host: 'sts.test-region.amazonaws.com' at port 'undefined'. This service may not be available in the 'test-region' region. . Please make sure that this role exists in the account. If it doesn't exist, (re)-bootstrap the environment with the right '--trust', using the latest version of the CDK CLI.

But I deployed stack manually and it has worked.

Thank you in advance!

Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

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.

@mrgrain
Copy link
Contributor

mrgrain commented Jun 19, 2023

Exemption Request

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Jun 19, 2023
@ViktarKhomich ViktarKhomich changed the title Exemption Request fix(servicecatalog): Make possible to have assets in different products in the same portfolio (#25189) Exemption Request Jun 19, 2023
@mrgrain mrgrain added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 26, 2023
@@ -30,7 +30,7 @@ export class ProductStackSynthesizer extends cdk.StackSynthesizer {
cdk.Annotations.of(parentStack).addWarning('[WARNING] Bucket Policy Permissions cannot be added to' +
' referenced Bucket. Please make sure your bucket has the correct permissions');
}
this.bucketDeployment = new BucketDeployment(parentStack, 'AssetsBucketDeployment', {
this.bucketDeployment = new BucketDeployment(parentStack, `${cdk.Names.uniqueId(this.boundStack)}-AssetsBucketDeployment`, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the bucket deployment can be shared within the same stack what if we did something like this:

this.bucketDeployment = parentStack.tryFindChild('AssetsBucketDeployment') ?? new BucketDeployment(...)

@corymhall corymhall removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 27, 2023
@corymhall
Copy link
Contributor

Hi Aws Cdk Team! Could you please run integration test for me? yarn integ test/aws-servicecatalog/test/integ.product.js --no-clean --update-on-failed

When I am doing it on my own, I am getting this error (region seems to be set in env variables):

It looks like this test might not be able to work with the update workflow. You can run the test with --disable-update-workflow or add the stackUpdateWorkflow: false to IntegTest.

@mrgrain
Copy link
Contributor

mrgrain commented Jul 7, 2023

@ViktarKhomich Happy to run the integ tests once you've addressed the previously received feedback.

@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.

@mrgrain mrgrain changed the title fix(servicecatalog): Make possible to have assets in different products in the same portfolio (#25189) Exemption Request fix(servicecatalog): Make possible to have assets in different products in the same portfolio Jul 12, 2023
@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Jul 18, 2023
@mrgrain mrgrain reopened this Jul 18, 2023
@mrgrain mrgrain added the pr-linter/do-not-close The PR linter will not close this PR while this label is present label Jul 18, 2023
@mrgrain mrgrain self-assigned this Jul 18, 2023
@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

… in the same portfolio (aws#25189)

Check for the existing AssetDeploymentBucket and if it is, use it otherwise create a new one
@kaizencc kaizencc reopened this Aug 9, 2023
@github-actions github-actions bot added effort/medium Medium work item – several days of effort p2 and removed p1 effort/large Large work item – several weeks of effort labels Aug 9, 2023
@kaizencc
Copy link
Contributor

kaizencc commented Aug 9, 2023

Hi @ViktarKhomich, are you still available to address feedback? We're happy to run the integ tests once you've addressed the previously received feedback.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@ViktarKhomich
Copy link
Author

Hi @kaizencc, yes, I am still available. I did changes regarding the comment above, but PR seems to be closed automatically again. Can we re-open it? Thank you!

@kaizencc kaizencc reopened this Aug 11, 2023
@kaizencc kaizencc added pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. labels Aug 11, 2023
@kaizencc
Copy link
Contributor

Reopened -- please make sure that the build succeeds and that you can merge from main to get the latest changes as well @ViktarKhomich

@aws-cdk-automation aws-cdk-automation dismissed their stale review August 11, 2023 16:13

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

@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Aug 12, 2023
mergify bot pushed a commit that referenced this pull request Aug 15, 2023
We have a label, `pr-linter/do-not-close` that has not been doing its job. See #26039.

It is because we are expecting a comma-separated-list, without spaces:

https://github.com/rix0rrr/close-stale-prs/blob/ffeb148adbaf7402e77bc4eafce8ed3d3c40f29c/src/index.ts#L22

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@kaizencc kaizencc reopened this Aug 15, 2023
@kaizencc
Copy link
Contributor

Okay I think I fixed the pr-linter/do-not-close label in a separate PR, and my hope is that this PR does not close automaticaly anymore. Sorry about the headache here

@kaizencc kaizencc removed the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Aug 25, 2023
@mrgrain mrgrain closed this Aug 25, 2023
@mrgrain
Copy link
Contributor

mrgrain commented Aug 25, 2023

Closing in favor of #26885

Thanks for you effort! Even though it didn't get merged in the end, it was useful for the other PR.

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 bug This issue is a bug. effort/medium Medium work item – several days of effort p2 pr-linter/do-not-close The PR linter will not close this PR while this label is present pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
5 participants