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
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
4 changes: 4 additions & 0 deletions packages/@aws-cdk/aws-ecr/lib/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,6 @@
// AWS::ECR CloudFormation Resources:
export * from './ecr.generated';

export * from './repository';
export * from './repository-ref';
export * from './lifecycle';
135 changes: 135 additions & 0 deletions packages/@aws-cdk/aws-ecr/lib/lifecycle.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
/**
* An ECR life cycle rule
*/
export interface LifecycleRule {
/**
* Controls the order in which rules are evaluated (low to high)
*
* When you add rules to a lifecycle policy, you must give them each a
* unique value for rulePriority. Values do not need to be sequential
* 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.


/**
* Describes the purpose of the rule
*
* @default No description
*/
description?: string;

/**
* Select images based on tags
*
* Only one rule is allowed to select untagged images, and it must
* have the highest rulePriority.
*
* @default TagStatus.Any
*/
tagStatus?: TagStatus;

/**
* Select images that have ALL the given prefixes in their tag.
*
* Only if tagStatus == TagStatus.Any
*/
tagPrefixList?: string[];

/**
* Specify limit type to apply to the repository
*/
countType: CountType;

/**
* 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?

*
* @default CountUnit.Days
*/
countUnit?: CountUnit;

/**
* The number of images or days, according to countType
*/
countNumber: number;

/**
* The action to take
*
* @default Action.expire
*/
action?: Action;
}

/**
* Select images based on tags
*/
export enum TagStatus {
/**
* Rule applies to all images
*/
Any = 'any',

/**
* Rule applies to tagged images
*/
Tagged = 'tagged',

/**
* Rule applies to untagged images
*/
Untagged = 'untagged',
}

/**
* Select images based on counts
*/
export enum CountType {
/**
* Set a limit on the number of images in your repository
*/
ImageCountMoreThan = 'imageCountMoreThan',

/**
* Set an age limit on the images in your repository
*/
SinceImagePushed = 'sinceImagePushed',
}

/**
* 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.

/**
* countNumber is in days
*/
Days = 'days',

/**
* 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#?

* occur when an enum only has one member.
*/
_ = '_',
}

/**
* Action to take on the image
*/
export enum Action {
/**
* The image is expired
*/
Expire = 'expire',

/**
* Do not use this value
*
* It is here to work around issues with the JSII type checker that
* occur when an enum only has one member.
*/
_ = '_',
}
80 changes: 80 additions & 0 deletions packages/@aws-cdk/aws-ecr/lib/repository-ref.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import cdk = require('@aws-cdk/cdk');
import { RepositoryArn, RepositoryName } from './ecr.generated';

/**
* An ECR repository
*/
export abstract class RepositoryRef extends cdk.Construct {
/**
* Import a repository
*/
public static import(parent: cdk.Construct, id: string, props: RepositoryRefProps): RepositoryRef {
return new ImportedRepository(parent, id, props);
}

/**
* The name of the repository
*/
public abstract readonly repositoryName: RepositoryName;

/**
* The ARN of the repository
*/
public abstract readonly repositoryArn: RepositoryArn;

public abstract addToResourcePolicy(statement: cdk.PolicyStatement): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

doc


/**
* Export this repository from the stack
*/
public export(): RepositoryRefProps {
return {
repositoryName: new RepositoryName(new cdk.Output(this, 'RepositoryName', { value: this.repositoryName }).makeImportValue()),
repositoryArn: new RepositoryArn(new cdk.Output(this, 'RepositoryArn', { value: this.repositoryArn }).makeImportValue()),
};
}

/**
* The URI of the repository, for use in Docker/image references
*/
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.

return new cdk.FnConcat(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not enjoy the wonders of token stringification and make this code nicer (we should avoid using cloudformation intrinsics if we can):

`${parts.account}.dkr.ecr.${parts.region}.amazonaws.com/${parts.resourceName}`

parts.account,
".dkr.ecr.",
parts.region,
".amazonaws.com/",
parts.resourceName
);
}
}

/**
* URI of a repository
*/
export class RepositoryUri extends cdk.CloudFormationToken {
}

export interface RepositoryRefProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

comments

Copy link
Contributor

Choose a reason for hiding this comment

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

If the ARN can be derived from the name (and/or vice versa), we should enable users to just specify one of the them.

repositoryName: RepositoryName;
repositoryArn: RepositoryArn;
}

/**
* An already existing repository
*/
class ImportedRepository extends RepositoryRef {
public readonly repositoryName: RepositoryName;
public readonly repositoryArn: RepositoryArn;

constructor(parent: cdk.Construct, id: string, props: RepositoryRefProps) {
super(parent, id);
this.repositoryName = props.repositoryName;
this.repositoryArn = props.repositoryArn;
}

public addToResourcePolicy(_statement: cdk.PolicyStatement) {
// FIXME: Add annotation about policy we dropped on the floor
}
}
138 changes: 138 additions & 0 deletions packages/@aws-cdk/aws-ecr/lib/repository.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
import cdk = require('@aws-cdk/cdk');
import { cloudformation, RepositoryArn, RepositoryName } from './ecr.generated';
import { Action, CountType, CountUnit, LifecycleRule, TagStatus } from './lifecycle';
import { RepositoryRef } from "./repository-ref";

export interface RepositoryProps {
/**
* Name for this repository
*
* @default Automatically generated name.
*/
repositoryName?: string;

/**
* Life cycle rules to apply to this registry
*
* @default No life cycle rules
*/
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?

*
* @default The default registry is assumed.
*/
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?

*
* If you don't set this to true, the registry must be empty, otherwise
* your stack deletion will fail.
*
* @default false
*/
retain?: boolean;
}

/**
* Define an ECR repository
*/
export class Repository extends RepositoryRef {
public readonly repositoryName: RepositoryName;
public readonly repositoryArn: RepositoryArn;
private readonly lifecycleRules = new Array<LifecycleRule>();
private readonly registryId?: string;
private policyDocument?: cdk.PolicyDocument;

constructor(parent: cdk.Construct, id: string, props: RepositoryProps = {}) {
super(parent, id);

const resource = new cloudformation.RepositoryResource(this, 'Resource', {
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?

});

if (props.retain) {
resource.options.deletionPolicy = cdk.DeletionPolicy.Retain;
}

this.registryId = props.lifecycleRegistryId;
if (props.lifecycleRules) {
props.lifecycleRules.forEach(this.addLifecycleRule.bind(this));
}

this.repositoryName = resource.ref;
this.repositoryArn = resource.repositoryArn;
}

public addToResourcePolicy(statement: cdk.PolicyStatement) {
if (this.policyDocument === undefined) {
this.policyDocument = new cdk.PolicyDocument();
}
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)

this.lifecycleRules.push(rule);
}

private renderLifecyclePolicy(): cloudformation.RepositoryResource.LifecyclePolicyProperty | undefined {
let lifecyclePolicyText: any;

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.

lifecyclePolicyText = JSON.stringify({
rules: this.lifecycleRules.map(renderLifecycleRule),
});
}

return {
lifecyclePolicyText,
registryId: this.registryId,
};
}
}

/**
* Render the lifecycle rule to JSON
*/
function renderLifecycleRule(rule: LifecycleRule) {
if (rule.tagStatus === TagStatus.Tagged && (rule.tagPrefixList === undefined || rule.tagPrefixList.length === 0)) {
throw new Error('TagStatus.Tagged requires the specification of a tagPrefixList');
}
if (rule.tagStatus !== TagStatus.Tagged && rule.tagPrefixList !== undefined) {
throw new Error('tagPrefixList can only be specified when tagStatus is set to Tagged');
}

if (rule.countType !== CountType.SinceImagePushed && rule.countUnit !== undefined) {
throw new Error('countUnit can only be specified when countType is set to SinceImagePushed');
}

if (rule.countUnit === CountUnit._) {
throw new Error('Do not use CountUnit._');
}

if (rule.action === Action._) {
throw new Error('Do not use Action._');
}

return {
rulePriority: rule.rulePriority,
description: rule.description,
selection: {
tagStatus: rule.tagStatus || TagStatus.Any,
tagPrefixList: rule.tagPrefixList,
countType: rule.countType,
countNumber: rule.countNumber,
countUnit: rule.countType === CountType.SinceImagePushed ? (rule.countUnit || CountUnit.Days) : undefined,
},
action: {
type: rule.action || Action.Expire
}
};
}
3 changes: 2 additions & 1 deletion packages/@aws-cdk/aws-ecr/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@
"@aws-cdk/assert": "^0.9.0",
"cdk-build-tools": "^0.9.0",
"cfn2ts": "^0.9.0",
"pkglint": "^0.9.0"
"pkglint": "^0.9.0",
"cdk-integ-tools": "^0.9.0"
},
"dependencies": {
"@aws-cdk/cdk": "^0.9.0"
Expand Down
Loading