-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Avoid pulling locally cached "infrastructure" (pause) image unless :latest tag is specified. Applies the same logic used for tasks to this image. #11380
Conversation
…atest tag is specified. Applies the same logic used for tasks to this image.
Happy to make changes to this to get it approved. I'm hoping it can make it into 1.2. |
I see the milestone for this was updated from 1.2.1 to 1.2.3. Is it likely to be included in that release? Thanks! |
The milestone it was originally added to was renamed from 1.2.1 to 1.2.3 because we had two emergency patch releases. For some reason this didn't get into the review queue, so I'll make sure someone (if not me) takes a look soon. |
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.
Hi @phreakocious! This is a great idea. I've let some comments about short-circuiting a check and some auth logic, but overall this is looking pretty good.
If you'd like to add a changelog entry for the improvement, that'd be great. But I can come in and clean that up if the changelog assistant workflow is too weird 😁 I pulled the PR off the milestone as we don't usually put PRs into milestones unless they're blockers for release, but if you get a chance to wrap up the changes before the holidays this will ship in 1.2.3.
repo, tag := parseDockerImage(d.config.InfraImage) | ||
|
||
// Pull infra_image if it doesn't already exist locally or tag 'latest' is specified | ||
if img, _ := client.InspectImage(d.config.InfraImage); img == nil || tag == "latest" { |
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.
Let's short-circuit the conditional by checking tag == "latest"
first so that we don't make a Docker API call if we don't have to.
I'm not wild about throwing away the error here either but it looks like (*Driver).pullImage
does it too... I guess if the inspect fails we always have a nil image and will try to pull, so any persistent errors talking to Docker will show up there.
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.
I'm relatively new to Go and not immediately seeing a way to reverse the order of the if
without moving its body to a new function. There is no way to move the assignment statement to the right of the ||
in that syntax. Is a new function what you'd suggest?
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.
I've sketched up https://play.golang.com/p/1IPLHG1NcwZ as an example. We reverse the sense of the conditionals and return early. So check tag != "latest"
, then check img != nil
and bail out if both are met. Then the fallthrough becomes where we pull the image.
adjust spacing Co-authored-by: Tim Gross <tgross@hashicorp.com>
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Running without internet connectivity requires a private registry to serve this image today and that is not always ideal. This provides some extra control over how it's image is handled and uses the same logic as tasks.
closes #10318
closes #11014