-
Notifications
You must be signed in to change notification settings - Fork 157
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
Added image tag option #204
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If i am not mistaken,
image.tags[0]
is ALWAYS the tag used to run the image. This is our design intentionally. Random tags make it an unreasonable automation and it might be better suited for you to use a shared tag as your run tag that has updates. This is all assuming that my initial statement is trueThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not my experience. For me, it was the last tag that was running. I had to untag the others before updates worked. While that isn't a big deal, it's an attempt to make the behaviour less random and more deterministic / explicit.
Regardless, this is merely an extension to the 'latest' feature; if that exists, I don't see a reason why it wouldn't be made slightly more flexible. If you only have the option to access this feature with the 'latest' tag, you can only use this on a single environment, but in my use case, I would like to use it on two (development and staging).
I would in fact argue that setting an image tag makes it less random, as I don't think image.tags[0] is reliable in the sense that I couldn't find any documentation that specifies how it is supposed to work. In other words, there don't seem to be any guarantees its behaviour will be backwards compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
image.tags[0] is not reliable with multiple tags (I have just tested). You are correct. but
container.attrs['Config']['Image']
Gives the exact tag used to run and that IS what our goal was. Im going to make a new pr with the above change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please check 55fb7b1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you of course. I do think it is inconsistent to leave in the 'latest' feature.
More importantly however, in your change, you did not address the neglected else in the pull method. The elif's are not exhaustive, causing an issue later on where the function returns None instead of raising an exception. I've seen this happen when trying to pull an image from gitlab.com; the unauthorized check didn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh im not a fan of latest existing at all. That is left from legacy code. Ill talk to the devs about removing it. As for the bug you mentioned, please capture the logs surrounding it so i can get more details and make an issue request for it and ill get it cleaned up :)