Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ECS private registry support design #1737
ECS private registry support design #1737
Changes from 6 commits
08980f5
9410746
f2f94a8
d7674ec
6ede80e
7877598
f2ed804
7c2d578
7c61d80
3c969fd
aee4606
c6f63a4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would get rid of this class and just call the secret grant directly in
ContainerImage
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind this rename at all, but being the pedant that I am, is "Web" the right term to use? It doesn't really have anything to do with a linked network of HTML pages...
InternetHostedImage
would be a more correct term.Not saying better, just more correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they default to being optional (with a
?
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since all properties will be optional, default to
props: WebHostedImageProps = {}
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we call this
fromRegistry
("registry" is the official term in the dockerverse)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ECR is also a registry but we don't want to include that. But maybe that ambiguity is okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is okay, especially given we will have both “fromEcr” and “fromRegistry” next to each other which disambiguates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering why we need this interface which adds another unneeded indirection instead of flat:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is in fact how it works. This interface is not used for input, it's for output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RegistryImage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you know what... this propery can probably be removed, no? I don't think it's used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(replying for context) You're right, it's a holdover from when we were checking it directly from
ContainerDefinition
to make a decision re: callingtoRepositoryCredentialsJson
, but once we moved to using multiple return types this check & field became unnecessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we call these methods
toRepositoryCredentialsJson
(JSON indicates it's low-level)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jsii should give you an error here because you changed the return type of this function while overriding it. C# does not allow narrowing the return type, so we have to disallow that everywhere as well.
You need to declare the return type as
CfnTaskDefinition.RepositoryCredentialsProperty | undefined
, even though we could give a narrower type.This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.