Add docker transport to push image before final failure #42
Conversation
|
@TomSweeneyRedHat We have a default transport in runtime.go (RuntimeConfig.ImageDefaultTransport) right now that can be set by the user, but it's not hooked into anything right now - can you either use that instead, or use that to set the value for DefaultTransport in runtime_img.go? |
|
@mheon I'd forgotten about the RuntimeConfig.ImageDefaultTransport . Will see what I can do with it, thanks! |
libpod/runtime_img.go
Outdated
There was a problem hiding this comment.
As long as you are at it, line 54ish in runtime.go has a struct that would benefit from this change too.
There was a problem hiding this comment.
@TomSweeneyRedHat Could you fix runtime.go to use this constant.
libpod/runtime_img.go
Outdated
There was a problem hiding this comment.
Depending on how you want to handle this @TomSweeneyRedHat , there is a slice for transports and we have a util.StringInSlice which takes a string and slice and returns a book depending on whether the string can be found in slice.
There was a problem hiding this comment.
or maybe it is time we break down and make an image-related function that is something like hasTransport ?
There was a problem hiding this comment.
I've made hasTransport, the stringinslice didn't fit as I've only a simple string at this point.
libpod/runtime_img.go
Outdated
There was a problem hiding this comment.
Ugh, looks like I've a merge mess. Will clean up tomorrow.
There was a problem hiding this comment.
fixed the merge mess.
TomSweeneyRedHat
left a comment
TomSweeneyRedHat
left a comment
There was a problem hiding this comment.
I think I've a mangled merge mess in runtime_img.go, will retry in the morning.
libpod/util.go
Outdated
There was a problem hiding this comment.
Why are you removing FuncTimer?
There was a problem hiding this comment.
Apparently my merge dance wasn't as smooth as it should have been. I didn't mean to remove that, back to dancing.
There was a problem hiding this comment.
Does the HasTransport function need to be public? I don't think it's that useful to people outside of libpod.
|
@mheon, thought I saw a comment about HasTransport being global. I can't find the comment, but have changed that in the code. You're probably right, I don't think it will be needed outside of the libpod. |
|
LGTM |
|
Test failures aren't lining up, going to close and reopen to force a test. |
|
@TomSweeneyRedHat gofmt errors. |
|
This PR hates me, looks like I've run into Travis environment issues. |
|
/test all |
|
OK, going to close and reopen to retest. Picked up a 503 on an unrelated test. |
|
Finally, all happy green buttons! |
|
📌 Commit c45706b has been approved by |
|
⌛ Testing commit c45706b with merge 2858100... |
|
💔 Test failed - status-papr |
|
@rh-atomic-bot retry |
|
⌛ Testing commit c45706b with merge ed6f58b... |
Signed-off-by: TomSweeneyRedHat <tsweeney@redhat.com> Closes: #42 Approved by: rhatdan
|
💔 Test failed - status-papr |
|
I think the CI PR that merged last night means this is no longer passing - @TomSweeneyRedHat can you rebase and repush to retrigger CI and see what's going on? |
|
@mheon will do and arg, this is a real problem child PR! |
|
For some reason Red Hat testing didn't like the if/else in hasTransports, have changed it to just a straight return. It passed prior, perhaps new testing in place for the builds? |
|
☔ The latest upstream changes (presumably #26) made this pull request unmergeable. Please resolve the merge conflicts. |
Signed-off-by: TomSweeneyRedHat <tsweeney@redhat.com>
|
@rhatdan my problem child PR is finally green, can we merge this? |
|
📌 Commit 6993881 has been approved by |
|
⌛ Testing commit 6993881 with merge dfe6038... |
|
💥 Test timed out |
|
@rh-atomic-bot retry |
|
☀️ Test successful - status-papr |
github-actions
bot
added
the
locked - please file new issue/PR
5 participants
Signed-off-by: TomSweeneyRedHat tsweeney@redhat.com
Add the transport "docker://" to the destination image if it fails and there's no transport specified. Then retry the lookup and finally fail if still not found. Also renamed the variable DefaultRegistry to DefaultTransport and replaced a few "docker://" in the code with the variable.