Merge Docker image_name into the repository field.
#13225
Conversation
Signed-off-by: Andreas Stenius <[email protected]> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
| @@ -78,50 +74,55 @@ def dockerfile_relpath(self) -> str: | |||
| def dockerfile_path(self) -> str: | |||
| return path.join(self.address.spec_path, self.dockerfile_relpath) | |||
|
|
|||
| def image_names( | |||
| def image_tags( | |||
There was a problem hiding this comment.
So this is in fact returning a full image "name" or "ref". The tag is just the bit at the end. So I'm not sure image_tags is a good method name here.
The best explanation I could find for thinking about Docker naming concepts is by analogy to git:
- Docker registry <==> Git server (e.g., github.com)
- Docker repository <==> Git repository
- Docker tag <==> Git tag
The terminology does make sense when we think about it that way (and it is probably implemented using Git under the covers...)
There does not appear to be one stable term for a full reference to an image of the form [<registry>/]<repository-name>[:<tag>]. By analogy to Git, this would be a "ref", but I have not seen that term used in this context. I do sometimes see reference to "name:tag" but not consistently. The Docker CLI help typically refers to this is just the "image", which isn't helpful.
So to sum up this long comment, I suggest we rename this method back to image_names, since that term is at least available for us to give meaning to.
There was a problem hiding this comment.
Yeah, I too have been struggling to find cohesive information wrgt image tags.
Based on what I found in the documentation for the --tag arg to docker build:
docker build -t vieux/apache:2.0 .
This will [...] tag the resulting image. The repository name will bevieux/apacheand the tag will be2.0
https://docs.docker.com/engine/reference/commandline/build/#tag-an-image--t
Here, they use the term repository name for what I interpret as the full image name (tag), without registry and version tag.
Following the link to docs for docker tag, they repeatedly refer to the part in front of the slash as the repository, i.e. vieux from the previous example.
However, the "formal" definition is very loosely described as:
An image name is made up of slash-separated name components, optionally prefixed by a registry hostname.
Leaving plenty of room for custom interpretations, such as for repository etc.
It is their use of "repository" and "repository name" that led me to use the field name "repository_name" for a field that would be used to set the <repository>/<name> part of the image. I would prefer simply repository, but then it feels wrong to tack a name in the value there..
Ah, hmm.. your analogy to git makes sense in the scope of github (or gitlab, etc) as they all share the concept of a "<project/user name>/" for repository, and by extension actually also to git itself. As if you only have local repos on disk, the name will have any arbitrary structure, but git will still refer to them as a repository.
That got to be a rather long reply, to end up simply agreeing with dropping the _name suffix on all repository_name. Thanks (that was what I wanted to begin with any way..)
The image_tags, I can see is easy to confuse with the tag part of the image name. Perhaps we can start our own trend, and refer to the full image tag/name as the "image ref", to avoid ambiguities. As dockers use tag is overloaded here. In that case, I suggest the method to be called image_refs instead, along with a little doc string explaining how that came to be, as to me image_name is not the full tag with the registry part.. could be, but it is no more clear than image_tag, and also, as the variable that picks up the result from the function is called tags, image_tags made sense in that scope.
| self, | ||
| default_name_template: str, | ||
| default_repository_name: str, |
There was a problem hiding this comment.
I suggest we s/repository[-_]name/repository/ throughout, and also for parity with the Docker docs/terminology.
| sub_repository="/".join( | ||
| [default_repo if self.repository.value else default_parent, repo] | ||
| ), | ||
| repository_name = repository_name.format( |
There was a problem hiding this comment.
I don't like overwriting locals like this, when the previous and new values have different meanings (the former is a format string, the latter is an already-formatted string). It makes the code less readable (e.g., if I log or debug-print it I might get confused). So maybe name the original repository_fmt and this repository (as discussed above)?
| class DockerRepository(StringField): | ||
| alias = "repository" | ||
| class DockerRepositoryNameField(StringField): | ||
| alias = "repository_name" |
There was a problem hiding this comment.
As mentioned earlier, I actually prefer DockerRepositoryField and repository here. "name" isn't part of the terminology and doesn't really add any information.
| "By default, it uses the directory name of the BUILD file for this target." | ||
| 'The repository name for the Docker image. e.g. "<repository>/<name>".\n\n' | ||
| "It uses the `[docker].default_repository_name` by default." | ||
| "This field value is formatted, see the documentation for " |
There was a problem hiding this comment.
"This field value may contain format strings that will be interpolated at runtime." ?
[ci skip-rust] [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
kaos
changed the title
image_name with repository into repository_name.image_name into the repository field.
) This gets rid of the `image_name` field in favour of using the `repository` field for the full image name. The corresponding `default_image_name_template` has been renamed `defualt_repository` and the `image_name_template` field has been removed, in favour of supporting format strings for the `repository` field. # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]
Simplifying the image name configuration options and fields. Fewer moving parts to document and maintain (while preserving the flexibility).
Reverted the default value for repository, to not include the directory name in the image tag, so that will be opt-in instead.