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

Accept image names with library/ prefix as a valid substitute #6174

Merged
merged 11 commits into from
Dec 14, 2022

Conversation

eddumelendez
Copy link
Member

@eddumelendez eddumelendez commented Nov 14, 2022

Docker official images can be pulled with or without library/
prefix as part of the image name. For example, mysql or
library/mysql. This is explicit when using private repositories.

Container classes such as CassandraContainer, for example, it
already considers as a valid substitution cassandra.
This change, by default adds a new substitution with library/
prefix.

Docker official images can be pulled with or without `library/`
prefix as part of the image name. For example, `mysql` or
`library/mysql`. This is explicit when using private repositories.
@bsideup
Copy link
Member

bsideup commented Nov 15, 2022

@eddumelendez have you considered having it in assertCompatibleWith instead? Would especially be helpful if the prefix will change to something else one day (since it is implicit in Docker) 👍

@kiview
Copy link
Member

kiview commented Nov 15, 2022

Thanks a lot for providing the context here in the PR @eddumelendez and thanks to @bsideup for more ideas.

My preferred way initially was to combine both changes (either within this PR, or 2 distinct PRs):

  1. Have assertCompatibleWith be aware of the library prefix for all images (indeed, we don't need to handle it for specific images)
  2. Internally specify images using library/ prefix, to solve issues such as [Enhancement]: Prepend "library/" to official docker image "alpine" #6166 or [Bug]: library/mysql is not a valid substitute for mysql #5612 in a very transparent way.

Reflecting on this, I see no drawback with doing 1), especially since it is a very local change. Regarding 2) I am now unsure. What binds is more strongly to Docker implementation details and is more prone to breaking, using mysql or library/mysql? I think it is still fine to integrate both changes, WDYT?

@bsideup
Copy link
Member

bsideup commented Nov 15, 2022

Internally specify images using library/ prefix

I am not sure about this one. This indeed exposes Docker's internal impl and, should they change it, we might be in trouble. I see no reason to touch how we define the images (redis:6 is an absolutely valid Docker image name, but I do see how treating library/-prefixed image names makes sense, because these are indeed compatible. So, can we please proceed with just 1?

@kiview
Copy link
Member

kiview commented Nov 17, 2022

@bsideup Yes makes sense, we will proceed with 1) only.

@eddumelendez eddumelendez added this to the next milestone Nov 17, 2022
@kiview
Copy link
Member

kiview commented Nov 28, 2022

I think the implementation becomes much more straightforward if moved to isCompatibleWith(). Something along the lines of:

     public boolean isCompatibleWith(DockerImageName other) {
         // is this image already the same or equivalent?
-        if (other.equals(this)) {
+        DockerImageName imageWithLibraryPrefix = DockerImageName.parse("library/" + other.repository);
+        if (other.equals(this) || imageWithLibraryPrefix.equals(this)) {
             return true;
         }
        [...]
     }

finalImageName = LIBRARY_PREFIX + this.repository;
}
DockerImageName imageWithLibraryPrefix = DockerImageName.parse(finalImageName);
if (other.equals(this) || imageWithLibraryPrefix.equals(this)) {
Copy link

Choose a reason for hiding this comment

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

(Minor) More cleaner

if (Objects.equals(other, this) || Objects.equals(imageWithLibraryPrefix, this)) {
   ....
}

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this changes the degree of "cleanness", but a question of taste (beside the null-safety of Objects.equals() in general). Either style should be fine in this case.

kiview
kiview previously approved these changes Dec 12, 2022
Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

LGTM, I much prefer the implementation in isCompatibleWith() 👍
See the open comment, I think we can merge once adressed.

finalImageName = LIBRARY_PREFIX + this.repository;
}
DockerImageName imageWithLibraryPrefix = DockerImageName.parse(finalImageName);
if (other.equals(this) || imageWithLibraryPrefix.equals(this)) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this changes the degree of "cleanness", but a question of taste (beside the null-safety of Objects.equals() in general). Either style should be fine in this case.

@@ -232,7 +234,14 @@ public DockerImageName asCompatibleSubstituteFor(DockerImageName otherImageName)
*/
public boolean isCompatibleWith(DockerImageName other) {
// is this image already the same or equivalent?
Copy link
Member

Choose a reason for hiding this comment

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

The comment makes no more sense here like this. Instead, we can add a comment saying:

// Make sure we always compare against a version of the image name containing the LIBRARY_PREFIX

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants