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

Allow CAPS in image tag #393

Merged
merged 1 commit into from
Oct 29, 2021
Merged

Allow CAPS in image tag #393

merged 1 commit into from
Oct 29, 2021

Conversation

hsuchan
Copy link
Contributor

@hsuchan hsuchan commented Oct 28, 2021

Only tags can have uppercase characters.
Excluding tags from lowercase comparison.

@phbelitz phbelitz changed the base branch from master to develop October 29, 2021 07:42
Copy link
Member

@phbelitz phbelitz left a comment

Choose a reason for hiding this comment

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

please add another test case in tests/test_image.py, or tell me if I should do the changes :) thank you very much for your support!

connaisseur/image.py Outdated Show resolved Hide resolved
@hsuchan
Copy link
Contributor Author

hsuchan commented Oct 29, 2021

please add another test case in tests/test_image.py, or tell me if I should do the changes :) thank you very much for your support!

I added a test case for tag using uppercase letter.

Copy link
Collaborator

@xopham xopham left a comment

Choose a reason for hiding this comment

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

@hsuchan lgtm!

Could you adjust the commit message to follow the semantic and conventional commits as described in the contributing guide. Should be something along the lines:

fix: allow CAPS in image tag

fixes #392 

We would then need rebase to develop again, sign it and could probably merge it.

Thanks for contributing 🙏

@xopham xopham linked an issue Oct 29, 2021 that may be closed by this pull request
@hsuchan
Copy link
Contributor Author

hsuchan commented Oct 29, 2021

I did rebase to develop. Let me know if I did it correctly.

"image",
"Tag",
None,
None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
None,
"library",

This needs to be the repository value which is "library" in your case, as given in line 8 though I admit the description is a bit confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just copying the previous test ("docker.io/Library/image:tag") and changed the test case.
I also noticed that the repository was set to "None" but reasoned that any Image under docker.io/library can be called by it's short name:

docker pull docker.io/library/nginx
docker pull library/nginx
docker pull nginx

Let me know if you want me to change the repository to "library"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the suggested change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see what you mean. That is, however, only a unit test that does not take the docker.io specifics into account. I agree that specifically choosing library as a repo name in the previous example is confusing though.

thanks, looks good now.

@hsuchan
Copy link
Contributor Author

hsuchan commented Oct 29, 2021

I also signed my commit.

@codecov-commenter
Copy link

Codecov Report

Merging #393 (63b84b7) into develop (8eab731) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #393   +/-   ##
========================================
  Coverage    96.68%   96.68%           
========================================
  Files           21       21           
  Lines         1057     1057           
========================================
  Hits          1022     1022           
  Misses          35       35           
Impacted Files Coverage Δ
connaisseur/image.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8eab731...63b84b7. Read the comment docs.

xopham
xopham previously approved these changes Oct 29, 2021
Copy link
Collaborator

@xopham xopham left a comment

Choose a reason for hiding this comment

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

@hsuchan great work 🙏
I briefly fixed the minor test issue. looks perfect now 🙂

@xopham xopham dismissed phbelitz’s stale review October 29, 2021 21:57

requested changes were implemented

xopham
xopham previously approved these changes Oct 29, 2021
Copy link
Collaborator

@xopham xopham left a comment

Choose a reason for hiding this comment

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

forgot to sign the commit previously 😅
will merge now 🚀

@xopham xopham merged commit 232e5b7 into sse-secure-systems:develop Oct 29, 2021
@hsuchan hsuchan deleted the fix/allow-caps-image-tag branch October 31, 2021 15:45
@phbelitz phbelitz mentioned this pull request Nov 23, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow CAPS in image tag
4 participants