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.

Sign up for GitHub

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

Jump to bottom

fix(cdk): fix a bug with CodePipelineSource to create nested repositories #27668

Closed
wants to merge 5 commits into from
Closed

fix(cdk): fix a bug with CodePipelineSource to create nested repositories #27668

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4ba68a5
fix(cdk): fix a bug with CodePipelineSource to create nested reposito…
anshikam Oct 24, 2023
3de98c6
Merge branch 'main' into branch_27504
anshikam Oct 24, 2023
1200929
Merge branch 'main' into branch_27504
anshikam Oct 25, 2023
cc78206
Merge branch 'main' into branch_27504
anshikam Oct 25, 2023
cebd303
Merge branch 'main' into branch_27504
anshikam Nov 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/aws-cdk-lib/pipelines/lib/codepipeline/codepipeline-source.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,11 +256,11 @@ class GitHubSource extends CodePipelineSource {
super(repoString);

const parts = repoString.split('/');
if (Token.isUnresolved(repoString) || parts.length !== 2) {
if (Token.isUnresolved(repoString) || parts.length < 2) {
throw new Error(`GitHub repository name should be a resolved string like '<owner>/<repo>', got '${repoString}'`);
Comment on lines +259 to 260
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets move this validation into a function. It should cover the edge case of a string such as 'owner/group1//repo' and give a specific error message.

}
this.owner = parts[0];
this.repo = parts[1];
this.repo = parts.slice(1).join('/');
this.authentication = props.authentication ?? SecretValue.secretsManager('github-token');
this.configurePrimaryOutput(new FileSet('Source', this));
}
Expand Down Expand Up @@ -425,11 +425,11 @@ class CodeStarConnectionSource extends CodePipelineSource {
super(`${repoString}-${branch}`);

const parts = repoString.split('/');
if (Token.isUnresolved(repoString) || parts.length !== 2) {
if (Token.isUnresolved(repoString) || parts.length < 2) {
throw new Error(`CodeStar repository name should be a resolved string like '<owner>/<repo>', got '${repoString}'`);
Comment on lines +428 to 429
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here.

Copy link

@davemeech davemeech Oct 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We might also consider changing the verbiage in the error message regarding the expected resolved string format. As an example, the same repository name field in the console expresses the format as group/subgroup/project.

}
this.owner = parts[0];
this.repo = parts[1];
this.repo = parts.slice(1).join('/');
this.configurePrimaryOutput(new FileSet('Source', this));
}

Expand Down
19 changes: 19 additions & 0 deletions packages/aws-cdk-lib/pipelines/test/codepipeline/codepipeline-sources.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,25 @@ test('Dashes in repo names are removed from artifact names', () => {
});
});

test('Nested repo names are allowed', () => {
new ModernTestGitHubNpmPipeline(pipelineStack, 'Pipeline', {
input: cdkp.CodePipelineSource.gitHub('owner/group1/group2/groupN/repo', 'main'),
});

Template.fromStack(pipelineStack).hasResourceProperties('AWS::CodePipeline::Pipeline', {
Stages: Match.arrayWith([{
Name: 'Source',
Actions: [
Match.objectLike({
OutputArtifacts: [
{ Name: 'owner_group1_group2_groupN_repo_Source' },
],
}),
],
}]),
});
});

test('artifact names are never longer than 128 characters', () => {
new ModernTestGitHubNpmPipeline(pipelineStack, 'Pipeline', {
input: cdkp.CodePipelineSource.gitHub('owner/' + 'my-repo'.repeat(100), 'main'),
Expand Down
Loading