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(codepipeline): Pipeline Variables #5604

Merged
merged 4 commits into from
Jan 15, 2020

Conversation

skinny85
Copy link
Contributor

@skinny85 skinny85 commented Jan 1, 2020

Added support for the CodePipeline Variables feature, by adding action class-specific interfaces that represent the collection of variables they emit, and a readonly property of the action instance that returns that interface. Plus a class representing the global pipeline variables with static properties.

Fixes #5219


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

@skinny85 skinny85 added the pr/do-not-merge This PR should not be merged at this time. label Jan 1, 2020
@skinny85 skinny85 requested a review from eladb January 1, 2020 01:34
@skinny85
Copy link
Contributor Author

skinny85 commented Jan 1, 2020

Submitted for early feedback.

Still missing: updating the ReadMe files.

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jan 1, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

*
* @see https://docs.aws.amazon.com/codepipeline/latest/APIReference/API_PutJobSuccessResult.html
*/
public variable(variableName: string): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might confuse people why this class has both variable and variableExpression, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

variableExpression is protected, it's not part of the public API of the class, so I don't think that will be an issue.

*
* @default - no variables will be emitted from this action
*/
readonly variables?: CodeCommitSourceVariables;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should just be a boolean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@skinny85 skinny85 force-pushed the feat/codepipeline-variables branch from 88ca213 to 1c96514 Compare January 2, 2020 23:18
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@jgondron
Copy link
Contributor

jgondron commented Jan 6, 2020

This is sort of a general question about validation expectations in cdk: Should this validate that the namespace given doesn't collide with an artifact name? When this happens, CloudFormation will catch it, throw an exception, and rollback etc, but seems like cdk should be proactive in validating this to fail faster. I did something similar and tested for it in https://github.com/aws/aws-cdk/pull/5326/files#diff-7cc3d22a6d7b4682f2c898e18e4e1e61R118, although you'd probably have a more elegant approach knowing the codebase. Obviously, this is mostly for the case where someone provides a namespace explicitly.

@skinny85
Copy link
Contributor Author

skinny85 commented Jan 9, 2020

This is sort of a general question about validation expectations in cdk: Should this validate that the namespace given doesn't collide with an artifact name? When this happens, CloudFormation will catch it, throw an exception, and rollback etc, but seems like cdk should be proactive in validating this to fail faster. I did something similar and tested for it in https://github.com/aws/aws-cdk/pull/5326/files#diff-7cc3d22a6d7b4682f2c898e18e4e1e61R118, although you'd probably have a more elegant approach knowing the codebase. Obviously, this is mostly for the case where someone provides a namespace explicitly.

Yep, that's a great point @jgondron!

@skinny85 skinny85 force-pushed the feat/codepipeline-variables branch from 95de96b to e7aff40 Compare January 10, 2020 04:01
@skinny85
Copy link
Contributor Author

Changed to have variables be read-only instance properties of the different action classes. @eladb , please re-review. Thanks!

@skinny85 skinny85 removed the pr/do-not-merge This PR should not be merged at this time. label Jan 10, 2020
@skinny85 skinny85 requested a review from eladb January 10, 2020 04:04
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Comment on lines 80 to 82
throw new Error(`Cannot reference variables of action '${this.actionProperties.actionName}', ` +
'as that action was never added to a pipeline');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of making the variable expression a lazy value, just make the actual namespace a lazy value. This way you can get rid of the error-prone variableWasReferenced method:

protected variableExpression(variableName: string): string {
  this.variableReferenced = true;
  return `#{${this.variablesNamespace}}.${variableName}`;
}

And then in the initializer:

this.variablesNamespace = actionProperties.variablesNamespace 
  ?? Lazy.string({ produce: () => this.renderedNamespace });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this API even more error-prone? If you decide to cache the variables, like I did originally:

export class GitHubSourceAction extends Action {
  constructor(props: GitHubSourceActionProps) {
    // ...
    this._variables = {
      repositoryName: this.variableExpression('RepositoryName'),
      branchName: this.variableExpression('BranchName'),
      authorDate: this.variableExpression('AuthorDate'),
      committerDate: this.variableExpression('CommitterDate'),
      commitId: this.variableExpression('CommitId'),
      commitMessage: this.variableExpression('CommitMessage'),
      commitUrl: this.variableExpression('CommitUrl'),
    };
  }

, instead of returning them on demand from the variables getter, you will always make variableReferenced true, even if no variables were actually referenced, and thus always render the namespace, even if it wasn't required.

@skinny85 skinny85 force-pushed the feat/codepipeline-variables branch from e7aff40 to bba4cdd Compare January 14, 2020 01:14
@skinny85
Copy link
Contributor Author

Changed to implement according to #5604 (comment).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Added support for the CodePipeline Variables feature,
by adding action class-specific interfaces that represent the
collection of variables they emit,
and a readonly property of the action instance that returns that interface.
Plus a class representing the global pipeline variables
with static properties.

Fixes aws#5219
@skinny85 skinny85 force-pushed the feat/codepipeline-variables branch from bba4cdd to 14b023b Compare January 14, 2020 21:59
@skinny85 skinny85 requested a review from eladb January 14, 2020 22:01
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@skinny85
Copy link
Contributor Author

Changed the names as suggested, @eladb this is ready for another round of reviews. Thanks!

@mergify
Copy link
Contributor

mergify bot commented Jan 15, 2020

Thank you for contributing! Your pull request is now being automatically merged.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Jan 15, 2020

Thank you for contributing! Your pull request is now being automatically merged.

@mergify mergify bot merged commit 34d3e7d into aws:master Jan 15, 2020
@skinny85 skinny85 deleted the feat/codepipeline-variables branch January 15, 2020 19:06
@gdottheking
Copy link

gdottheking commented May 4, 2022

CloudFormationAction (and subclasses) still do not have a variable() method in cdk 2.22.
Is this under consideration?

@skinny85
Copy link
Contributor Author

skinny85 commented May 4, 2022

Yes, it definitely is (I can't find the issue for it, but it is).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for CodePipeline variables
5 participants