-
Notifications
You must be signed in to change notification settings - Fork 4k
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(codepipeline-actions): use token as CodeCommitSourceAction branch #10463
Conversation
When using the EVENTS trigger, an event is created based on the branch name of the event, however this is not possible if the branch name is an unresolved value. Therefore generate a unique event name if this is the case. Fixes aws#10263
ca32bea
to
6131de4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @alanraison ! There's a bug lurking in the current proposal, so we need to address it before merging it in 🙂
private generateEventId(stage: codepipeline.IStage): string { | ||
let branchIdDisambiguator: string; | ||
let baseId = stage.pipeline.node.uniqueId; | ||
if (this.branch === 'master') { | ||
branchIdDisambiguator = baseId; | ||
} else if (Token.isUnresolved(this.branch)) { | ||
let candidate = baseId; | ||
let counter = 0; | ||
while (this.props.repository.node.tryFindChild(candidate) !== undefined) { | ||
counter += 1; | ||
candidate = `${baseId}-Branch${counter}-`; | ||
} | ||
branchIdDisambiguator = candidate; | ||
} else { | ||
branchIdDisambiguator = `${baseId}-${this.branch}-`; | ||
} | ||
return `${branchIdDisambiguator}EventRule`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a hard time keeping up with this code. For example, branchIdDisambiguator
sounds to me like it should be just the branch-changing part of the ID, but somehow it's the entire thing.
What do you think of this instead?
private generateEventId(stage: codepipeline.IStage): string { | |
let branchIdDisambiguator: string; | |
let baseId = stage.pipeline.node.uniqueId; | |
if (this.branch === 'master') { | |
branchIdDisambiguator = baseId; | |
} else if (Token.isUnresolved(this.branch)) { | |
let candidate = baseId; | |
let counter = 0; | |
while (this.props.repository.node.tryFindChild(candidate) !== undefined) { | |
counter += 1; | |
candidate = `${baseId}-Branch${counter}-`; | |
} | |
branchIdDisambiguator = candidate; | |
} else { | |
branchIdDisambiguator = `${baseId}-${this.branch}-`; | |
} | |
return `${branchIdDisambiguator}EventRule`; | |
} | |
private generateEventId(stage: codepipeline.IStage): string { | |
const baseId = stage.pipeline.node.uniqueId; | |
if (Token.isUnresolved(this.branch)) { | |
let candidate = ''; | |
let counter = 0; | |
do { | |
candidate = this.eventIdFromPrefix(`${baseId}-${counter}`); | |
counter += 1; | |
} while (this.props.repository.node.tryFindChild(candidate) !== undefined); | |
return candidate; | |
} else { | |
const branchIdDisambiguator = this.branch === 'master' ? '' : '-${this.branch}-'; | |
return this.eventIdFromPrefix(`${baseId}${branchIdDisambiguator}`); | |
} | |
} | |
private eventIdFromPrefix(eventIdPrefix: string) { | |
return `${eventIdPrefix}EventRule`; | |
} |
(Notice the different loop; the current one actually checks the wrong ID, without the EvenRule
suffix!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, thanks for spotting that! I think that your naming scheme with the counter needs a slight tweak though, so I will just update my branch with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/@aws-cdk/aws-codepipeline-actions/test/codecommit/test.codecommit-source-action.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-codepipeline-actions/test/codecommit/test.codecommit-source-action.ts
Outdated
Show resolved
Hide resolved
Cleaning up the tests Co-authored-by: Adam Ruka <[email protected]>
Pull request has been modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix @alanraison !
Pull request has been modified.
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
When using the EVENTS trigger, an event is created based on the branch name of
the event, however this is not possible if the branch name is an unresolved
value. Therefore generate a unique event name if this is the case.
Fixes #10263
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license