[EUWE] Identifying container images by registry, name and digest #14396
Conversation
|
cc @simaishi |
|
@miq-bot assign blomquisg |
|
LGTM 👍 |
|
@enoodle please rename this with "[EUWE]" in the title of the PR. |
enoodle
changed the title
|
@blomquisg please approve if this is good to go. |
|
@miq-bot assign blomquisg |
Oops sorry @chessbyte I didn't notice you reassigned this already. |
app/models/container_image.rb
Outdated
| @@ -36,6 +36,7 @@ class ContainerImage < ApplicationRecord | |||
| after_create :raise_creation_event | |||
|
|
|||
| def full_name | |||
| docker_id if image_ref && image_ref.start_with?(DOCKER_PULLABLE_PREFIX) | |||
There was a problem hiding this comment.
This looks like a no-op line to me. It will evaluate docker_id, but docker_id isn't actually used in this method anywhere.
Did you mean to return docker_id?
EDIT:
The best part is I merged this same change upstream...
There was a problem hiding this comment.
@blomquisg 😮 kudos!
@enoodle please fix this here and on master as well.
|
@enoodle one comment about the |
enoodle
force-pushed
the
better_identify_container_images_euwe
branch
from
a92f8dd
to
51a4108
Compare
| @@ -36,6 +36,7 @@ class ContainerImage < ApplicationRecord | |||
| after_create :raise_creation_event | |||
|
|
|||
| def full_name | |||
| return docker_id if image_ref && image_ref.start_with?(DOCKER_PULLABLE_PREFIX) | |||
There was a problem hiding this comment.
@enoodle how come the tests were not failing before? We're not covering this with a test?
|
@blomquisg I will add a test here, will ping you back in 30 minutes :) |
enoodle
changed the title
The image-ref is not a reliable source to identify images when it comes from the docker daemon (in the docker://... form). This will identify the images from the information that we can parse. Doing so will also enable us to commit the images to the @DaTa hash when we identify the images instead from a collection function (get_images), which will simplify image collection from Openshift.
enoodle
force-pushed
the
better_identify_container_images_euwe
branch
from
51a4108
to
931d0ab
Compare
|
Checked commit enoodle@931d0ab with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb
spec/models/manageiq/providers/openshift/container_manager/refresh_parser_spec.rb
|
enoodle
changed the title
|
@blomquisg Ok, I fixed the problem with |
|
LGTM 👍 cc @blomquisg |
|
@enoodle can you submit the same change to the |
|
@blomquisg Yes, I will link it here in a few minutes |
|
Thanks @enoodle!! |
The image-ref is not a reliable source to identify images when it comes
from the docker daemon (in the docker://... form). This will identify
the images from the information that we can parse.
Doing so will also enable us to commit the images to the @DaTa hash when
we identify the images instead from a collection function (get_images),
which will simplify image collection from Openshift.
This is a backport to euwe of #14185