feat: Implement CodeArtifact L2 construct #11091
Conversation
|
|
There was a problem hiding this comment.
Thanks for the contribution! Tackling an L2 is always a fun challenge. Here's my first pass of comments.
Please add @experimental to the docstrings all of the exported interfaces/classes.
General comment on styling/formatting. This package is currently missing the .eslintrc.js file which would bring up a lot of formatting/spacing/styling changes. Can you please add (just copy/paste from another module) and clean up?
|
Ready for 2nd review |
| ```ts | ||
| import * as codeartifact from '@aws-cdk/aws-codeartifact'; | ||
|
|
||
| const domain = new codeartifact.Domain(stack, 'domain', { domainName: 'example-domain' }); |
There was a problem hiding this comment.
| const domain = new codeartifact.Domain(stack, 'domain', { domainName: 'example-domain' }); | |
| new codeartifact.Domain(stack, 'domain', { domainName: 'example-domain' }); |
| const domain = new codeartifact.Domain(stack, 'domain', { domainName: 'example-domain' }); | ||
| const repository = new codeartifact.Repository(stack, 'repository', { | ||
| repositoryName: 'repository', | ||
| domain: domain, |
There was a problem hiding this comment.
| domain: domain, | |
| domain, |
| const domain = new codeartifact.Domain(stack, 'domain', { domainName: 'example-domain' }); | ||
| const repo1 = new codeartifact.Repository(stack, 'repository_1', { repositoryName: 'repository-1' }); | ||
| const repo2 = new codeartifact.Repository(stack, 'repository_2', { repositoryName: 'repository-2' }); | ||
|
|
||
| domain.addRepositories(repo1, repo2); |
There was a problem hiding this comment.
Take a look at https://github.com/aws/aws-cdk/blob/master/DESIGN_GUIDELINES.md#factories again. Ideally, this addRepository method takes in a RepositoryOption interface, which is a sub-interface of RepositoryProps.
export interface RepositoryOptions {
readonly repositoryName: string,
readonly description?: string,
...
}
export interface RepositoryProps extends RepositoryOptions {
readonly domain: IDomain;
}
// THEN
const domain = new codeartifact.Domain(stack, 'Domain');
domain.addRespository({ repositoryName: 'MyRepo', description: 'NPM + Java repo' });| ``` | ||
|
|
||
| ### External Connections | ||
| Extenal connections can be added by calling `.withExternalConnections(...)` method on a repository. It accepts and |
There was a problem hiding this comment.
Can you add a sentence (or two) here (and for Upstream Repositories) just explaining what they are? Also, I still think we should defer withExternalConnections until we have a use case.
| domainName: props.domain?.domainName || '', | ||
| repositoryName: props.repositoryName, | ||
| description: props.description, | ||
| upstreams: props.upstreams ? props.upstreams.map(u => u.repositoryName) : [] as string[], |
There was a problem hiding this comment.
| upstreams: props.upstreams ? props.upstreams.map(u => u.repositoryName) : [] as string[], | |
| upstreams: props.upstreams?.map(u => u.repositoryName), |
| } | ||
|
|
||
| if (rule.pattern && !rule.pattern.test(value)) { | ||
| throw new Error(`${name}: must match pattern ${rule.pattern.source}. Must match rules from ${rule.documentationLink}`); |
There was a problem hiding this comment.
Adding the documentationLink here I don't think adds a ton of value, since the pattern is already in the message.
packages/@aws-cdk/cfnspec/spec-source/000_CloudFormationResourceSpecification.json
Outdated
Show resolved
Hide resolved
njlynch
added
the
pr/requires-two-approvers
mergify
bot
dismissed
njlynch’s stale review
Pull request has been modified.
packages/@aws-cdk/cfnspec/spec-source/000_CloudFormationResourceSpecification.json
Outdated
Show resolved
Hide resolved
|
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Can you take another pass? I see that most of my comments from the last round of feedback have still not been addressed. |
|
Hi jstandish, For tracking sake, I'm going to close this PR, as we haven't made any progress in a while. When you're ready to pick this back up, feel free to mention me to re-open this, or just create a new PR. Thanks! |
#11090
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license