Fix docker_container.tag to use last element of image #2052
Conversation
lib/resources/docker_container.rb
Outdated
| @@ -78,7 +78,7 @@ def repo | |||
| end | |||
|
|
|||
| def tag | |||
| image.split(':')[1] unless image.nil? | |||
| image.split(':')[-1] unless image.nil? || !image.index(':') | |||
There was a problem hiding this comment.
@jerryaldrichiii added the suggestion to change this from index to include? :)
There was a problem hiding this comment.
Splitting on the colon like this could potentially introduce a problem if a non-default repohost was present but no tag was present. For example, if we received docker ps output that included: repo.example.com:5000/ubuntu, the tag we'd return would be 5000/ubuntu
I'd rather see us treat the non-default repo host as a standalone unit and split on /, and we need to fix both #repo and #tag... perhaps something like this:
def repo
return if image_name_from_image.nil?
image_name_from_image.split(':')[0]
end
def tag
return if image_name_from_image.nil?
image_name_from_image.split(':')[1]
end
def image_name_from_image
return if image.nil?
# possible image names include:
# alpine
# ubuntu:14.04
# repo.example.com:5000/ubuntu
# repo.example.com:5000/ubuntu:1404
image.include?('/') ? image.split('/')[1] : image
endWhat do you think, @mattlqx?
There was a problem hiding this comment.
Hi @mattlqx and thanks for your PR!
Thanks for finding this bug and proposing a fix. I have a small concern about splitting on a colon for a particular edge case, and I've left a comment accordingly. Please let me know what you think.
lib/resources/docker_container.rb
Outdated
| @@ -78,7 +78,7 @@ def repo | |||
| end | |||
|
|
|||
| def tag | |||
| image.split(':')[1] unless image.nil? | |||
| image.split(':')[-1] unless image.nil? || !image.index(':') | |||
There was a problem hiding this comment.
Splitting on the colon like this could potentially introduce a problem if a non-default repohost was present but no tag was present. For example, if we received docker ps output that included: repo.example.com:5000/ubuntu, the tag we'd return would be 5000/ubuntu
I'd rather see us treat the non-default repo host as a standalone unit and split on /, and we need to fix both #repo and #tag... perhaps something like this:
def repo
return if image_name_from_image.nil?
image_name_from_image.split(':')[0]
end
def tag
return if image_name_from_image.nil?
image_name_from_image.split(':')[1]
end
def image_name_from_image
return if image.nil?
# possible image names include:
# alpine
# ubuntu:14.04
# repo.example.com:5000/ubuntu
# repo.example.com:5000/ubuntu:1404
image.include?('/') ? image.split('/')[1] : image
endWhat do you think, @mattlqx?
Fixes #2051 Images with repos containing port numbers will have multiple colons. Signed-off-by: Matt Kulka <mkulka@parchment.com>
|
Thanks for bringing up this additional case. Your suggestion works well, so let's go with that. I've added an additional test case for it as well. |
* Fix docker_container.tag to properly fetch from image name (inspec#2052) Fixes inspec#2051 Images with repos containing port numbers will have multiple colons. Signed-off-by: Matt Kulka <mkulka@parchment.com> * Bump version to 1.32.3 by Chef Expeditor * Bump project minor version, bump train dependency version (inspec#2058) Bumping InSpec's minor version to 1.33 because a recent PR added new functionality. Also bumping train to 0.26 to pick up a recent bug fix. Signed-off-by: Adam Leff <adam@leff.co> * Bump version to 1.33.1 by Chef Expeditor * Update CHANGELOG.md to reflect the promotion of 1.33.1 to stable
Fixes #2051
Images with repos containing port numbers will have multiple colons.
Signed-off-by: Matt Kulka mkulka@parchment.com