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(aws-ecr): add support for ECR repositories #697

Merged
merged 3 commits into from
Sep 13, 2018
Merged

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Sep 12, 2018

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

@rix0rrr rix0rrr requested a review from eladb September 12, 2018 16:11
* 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue#?

/**
* The unit to count time in
*/
export enum CountUnit {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we even need this enum if it only has a single member?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@rix0rrr rix0rrr Sep 13, 2018

Choose a reason for hiding this comment

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

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())),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to explicitly resolve? Aren't tokens recursively resolved?

lifecycleRegistryId?: string;

/**
* Retain the registry on stack deletion
Copy link
Contributor

Choose a reason for hiding this comment

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

You mean "the repository", right?

lifecycleRules?: LifecycleRule[];

/**
* The AWS account ID associated with the registry that contains the repository.
Copy link
Contributor

Choose a reason for hiding this comment

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

"the lifecycle registry"

Can you add a @see link?

this.policyDocument.addStatement(statement);
}

public addLifecycleRule(rule: LifecycleRule) {
Copy link
Contributor

Choose a reason for hiding this comment

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

doc (copy & paste from AWS docs)


if (this.lifecycleRules.length === 0 && !this.registryId) { return undefined; }

if (this.lifecycleRules.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does it means to specify a registry ID without rules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, but I don't want to preclude it.

@rix0rrr rix0rrr merged commit c6c09bf into master Sep 13, 2018
@rix0rrr rix0rrr deleted the huijbers/ecr branch September 13, 2018 15:34
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
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.

3 participants