feat(ecs): Add RepositoryCredentials support #1
Conversation
|
Feel free to just post these PRs directly to the aws-cdk repository. It's actually easier for me to go over them there. |
| @@ -183,8 +183,8 @@ const taskDefinition = new ecs.TaskDefinition(this, 'TaskDef', { | |||
| Images supply the software that runs inside the container. Images can be | |||
| obtained from either DockerHub or from ECR repositories, or built directly from a local Dockerfile. | |||
|
|
|||
| * `ecs.ContainerImage.fromDockerHub(imageName)`: use a publicly available image from | |||
| DockerHub. | |||
| * `ecs.ContainerImage.fromInternet(imageName, { credentials?: MY_SECRET })`: use a public or private image from | |||
There was a problem hiding this comment.
I wouldn't write it like this, this isn't copy/pasteable user code. Just add a new line below to do something like:
ecs.ContainerImage.fromInternet(imageName, { credentials: mySecret }): use a private image that needs credentials.
| /** | ||
| * Repository Credential resources | ||
| */ | ||
| export interface RepositoryCreds { |
There was a problem hiding this comment.
What are your thoughts for this type? Future proofing? Because it's not strictly necessary today, right?
There was a problem hiding this comment.
Correct, today we could potentially add a secret to ContainerImage directly but I want to minimize the need for rewiring if other resources or additional fields become valid in the future. Do you foresee issues with this approach?
| /** | ||
| * Optional credentials for a private image registry | ||
| */ | ||
| public abstract readonly credentials?: RepositoryCreds; |
There was a problem hiding this comment.
Does this need to be publicly accessible? Seems like renderRepositoryCredentials() is all that a consumer of this class should need from it, right?
There was a problem hiding this comment.
renderContainerDefinition() needs to see it to determine whether or not to call renderRepositoryCredentials(). ECS will fail to register the task definition if RepositoryCredentials is present but empty.
| */ | ||
| public renderRepositoryCredentials(): CfnTaskDefinition.RepositoryCredentialsProperty { | ||
| return { | ||
| credentialsParameter: this.credentials ? this.credentials.secret.secretArn : '' |
There was a problem hiding this comment.
To avoid breaking all existing integration tests, maybe return undefined for this whole object if we don't have credentials?
public renderRepositoryCredentials(): CfnTaskDefinition.RepositoryCredentialsProperty | undefined {
if (!this.credentials) { return undefined; }
return {
credentialsParameter: this.credentials.secret.secretArn
} ;
}There was a problem hiding this comment.
And in fact, this method could be abstract here and only be implemented in InternetHostedImage, since that's the only image type that actually has any credentials at all.
There was a problem hiding this comment.
Well the others would need to do a base implementation of undefined as well.
There was a problem hiding this comment.
To avoid breaking all existing integration tests...
Ah, the conditional here can probably be removed b/c this func should only be called if credentials is populated. See renderContainerDefinition(). Currently, the existing integ tests all pass.
...this method could be
abstracthere and be implemented inInternetHostedImage...
I tried returning undefined from this method before (as you show) and got yelled at by the compiler b/c it wasn't a return value of type RepositoryCredentialsProperty. So since I have to return a RepositoryCredentialsProperty obj regardless, I'd prefer to consolidate it into one func on ContainerImage. Is there a way around having to return an RepositoryCredentialsProperty in some cases?
There was a problem hiding this comment.
got yelled at by the compiler
It's hard to see the change in the code I pasted because it scrolls off the side of the comment box, but you should have written the return type as:
CfnTaskDefinition.RepositoryCredentialsProperty | undefinedThat would have allowed you to return undefined.
There was a problem hiding this comment.
Ah, I totally missed that, thanks for pointing it out! In that case I'll update it per the above.
|
BTW, is it still possible to merge this PR into aws#1737 ? |
|
@rix0rrr sure! I'll address the above feedback and add it to awslabs#1737 |
|
was merged as aws#1737 |
Implements #1737
Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.