feat(aws-ecr): add support for ECR repositories #697
Conversation
| * across rules in a policy. A rule with a tagStatus value of any must have | ||
| * the highest value for rulePriority and be evaluated last. | ||
| */ | ||
| rulePriority: number; |
There was a problem hiding this comment.
I guess the common case would be to define a lifecycle rule for all images (tagStatus == any), in which case rulePriority must be the maximum value out of all rules). Perhaps if rulePriority is undefined, it will automatically get a priority allocated for it (max(rules)+1) or something. This way, we can make this optional and the common case more ergonomic.
There was a problem hiding this comment.
Yeah, I wasn't really sure of how to do this in an elegant way yesterday, but I think we can make both work.
| /** | ||
| * The unit of specifying the count | ||
| * | ||
| * Only applies when countType = CountType.SinceImagePushed. |
There was a problem hiding this comment.
Can we make countType implicit if countUnit is set?
| /** | ||
| * Do not use this value | ||
| * | ||
| * It is here to work around issues with the JSII type checker that |
| /** | ||
| * The unit to count time in | ||
| */ | ||
| export enum CountUnit { |
There was a problem hiding this comment.
Why do we even need this enum if it only has a single member?
There was a problem hiding this comment.
Yeah, I was on the fence about that as well myself. My thought was we'd automatically support new enum members as soon as they get added upstream (even though we might lack syntactical support).
But I guess that doesn't hold over JSII anyway.
| */ | ||
| public get repositoryUri(): RepositoryUri { | ||
| // Calculate this from the ARN | ||
| const parts = cdk.Arn.parseToken(this.repositoryArn); |
There was a problem hiding this comment.
Would be nice to support isToken(string) and then cdk.Arn.parse(s) will decide if it wants to parse this as a token or as a string
There was a problem hiding this comment.
Not sure that makes sense. The outputs of this function are definitely not interpretable from a CDK app (even though you can't tell from the types today, but you should be able to tell that). If it makes a decision at runtime to either return readable types or not, you're bound to make errors against that.
| repositoryName: props.repositoryName, | ||
| // It says "Text", but they actually mean "Object". | ||
| repositoryPolicyText: this.policyDocument, | ||
| lifecyclePolicy: new cdk.Token(() => cdk.resolve(this.renderLifecyclePolicy())), |
There was a problem hiding this comment.
Why do we need to explicitly resolve? Aren't tokens recursively resolved?
| lifecycleRegistryId?: string; | ||
|
|
||
| /** | ||
| * Retain the registry on stack deletion |
There was a problem hiding this comment.
You mean "the repository", right?
| lifecycleRules?: LifecycleRule[]; | ||
|
|
||
| /** | ||
| * The AWS account ID associated with the registry that contains the repository. |
There was a problem hiding this comment.
"the lifecycle registry"
Can you add a @see link?
| this.policyDocument.addStatement(statement); | ||
| } | ||
|
|
||
| public addLifecycleRule(rule: LifecycleRule) { |
There was a problem hiding this comment.
doc (copy & paste from AWS docs)
|
|
||
| if (this.lifecycleRules.length === 0 && !this.registryId) { return undefined; } | ||
|
|
||
| if (this.lifecycleRules.length > 0) { |
There was a problem hiding this comment.
What does it means to specify a registry ID without rules?
There was a problem hiding this comment.
I don't know, but I don't want to preclude it.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.