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(pipelines): Expose stack output namespaces in custom pipelines.Steps #23110

Conversation

tobni
Copy link
Contributor

@tobni tobni commented Nov 27, 2022

Closes #23000 as per request from #23000 (comment).

In order for custom steps to include CfnOutput in their action configurations, there needed to be a way to access and/or generate the output variable namespaces. ShellStep already had this capability.

This change generalizes the StackOutputReference usages by letting implementors of Step define the StackOutputReferences their step consumes, instead of hardwiring it to ShellStep.

To actually consume the references, the ICodePipelineActionFactory provides a StackOutputsMap that exposes a method to
render StackOutputReferences into their assigned CodePipeline variable names.


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 27, 2022

@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Nov 27, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team November 27, 2022 23:48
@github-actions github-actions bot added the p2 label Nov 27, 2022
@tobni tobni force-pushed the pipelines/enable-stack-output-namespaces-in-custom-steps branch from 43bed9c to 9ec69ac Compare November 27, 2022 23:50
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.

@tobni tobni force-pushed the pipelines/enable-stack-output-namespaces-in-custom-steps branch from 9ec69ac to fd37e4e Compare November 28, 2022 01:04
@aws-cdk-automation aws-cdk-automation dismissed their stale review November 28, 2022 01:06

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

Copy link
Contributor

@HBobertz HBobertz left a comment

Choose a reason for hiding this comment

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

Seems like a pretty simple feature to me, and looks like it's working as intended. In my opinion this feels like quite a lot of effort for not an immense payoff compared to the workaround. As I am new on the team I am going to run this by another engineer before approving

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Love this change! I'd like you to change a couple of small things, but overall thanks for generalizing this feature. Approved if you address my comments.

(Please do write a description of the motivation, the change and significant decisions made in the PR body though! Linking to the issue is not sufficient to cover the last 2 parts.)

packages/@aws-cdk/pipelines/lib/blueprint/shell-step.ts Outdated Show resolved Hide resolved
actionName: options.actionName,
runOrder: options.runOrder,
// Map the reference to the variable name the CDK has generated for you.
userParameters: {stackOutput: options.stackOutputsMap.mapOutputReference(this.stackOutputReference)},
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't compile, since stackOutputsMap is optional (should require a ?. or !.), but I'm pretty sure our code translator would balk at that.

Since ProduceActionOptions is much more of an output struct than an input struct, you have my permission to make the field required and silence the backwards compatibility checker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the compat error to 5bd7d2c, is that sufficient?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is it exactly

@tobni tobni force-pushed the pipelines/enable-stack-output-namespaces-in-custom-steps branch from 8c30b18 to b238b26 Compare December 1, 2022 10:02
@tobni
Copy link
Contributor Author

tobni commented Dec 1, 2022

Love this change! I'd like you to change a couple of small things, but overall thanks for generalizing this feature. Approved if you address my comments.

(Please do write a description of the motivation, the change and significant decisions made in the PR body though! Linking to the issue is not sufficient to cover the last 2 parts.)

I've addressed the comments (unsure if the compat check is correctly handled) and updated the PR description now.

@tobni tobni requested a review from rix0rrr December 1, 2022 14:41
rix0rrr
rix0rrr previously approved these changes Dec 1, 2022
@mergify
Copy link
Contributor

mergify bot commented Dec 1, 2022

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

@tobni
Copy link
Contributor Author

tobni commented Dec 2, 2022

@rix0rrr I don't understand the Mergify error https://github.com/aws/aws-cdk/runs/9825660539, is there some configuration/permission missing in my fork, or is it permissions in this repo that is sad?

@mergify mergify bot dismissed rix0rrr’s stale review December 2, 2022 09:02

Pull request has been modified.

@tobni tobni requested a review from rix0rrr December 5, 2022 10:24
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@rix0rrr rix0rrr enabled auto-merge (squash) January 5, 2023 14:29
rix0rrr
rix0rrr previously approved these changes Jan 5, 2023
@mergify mergify bot dismissed rix0rrr’s stale review January 5, 2023 14:30

Pull request has been modified.

@github-actions github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. labels Jan 5, 2023
@rix0rrr rix0rrr merged commit 14f6811 into aws:main Jan 5, 2023
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 34eb6ce
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@tobni tobni deleted the pipelines/enable-stack-output-namespaces-in-custom-steps branch January 11, 2023 13:48
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 effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(pipelines): Support generating stack output namespaces in custom Steps
4 participants