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

Set tag when pulling image missing in local storage #96

Merged
merged 4 commits into from
Apr 5, 2021

Conversation

pabloyoyoista
Copy link
Contributor

@pabloyoyoista pabloyoyoista commented Apr 4, 2021

First of all, thank you very much for this plugin!

I was testing some basic configuration with nomad and this plugin and bumped into the following issue:

With a clean podman installation, I tried to run the following task, which failed with task=apache error="rpc error: code = Unknown desc = failed to start task, could not create container: unknown error, status code: 500: {"cause":"no such image","message":"unable to find 'docker.io/httpd:2' in local storage: no such image","response":500}"

task "apache" {
  driver = "podman"

  config {
    image = "docker.io/httpd:2"
    ports = ["http"]
  }
}

To my surprise, the image saved to storage was httpd:latest instead of httpd:2. I understand the reason is that podman interprets the missing tag as latest when asking ImagePull just for the imageName

This PR fixes this issue and adds tag information to the logging and error in case the image is not present in local storage. I also understand that this should be the expected behavior according to the docker driver documentation.

@hashicorp-cla
Copy link

hashicorp-cla commented Apr 4, 2021

CLA assistant check
All committers have signed the CLA.

@towe75
Copy link
Collaborator

towe75 commented Apr 4, 2021

@pablofsf thank you for this PR, it's very much appreciated! The bug is a regression of #88 and it would be nice to cleanup a bit more.

There are now several identical Sprintf(name,tag) invocations in consecutive lines. It would be better to format the string once and use it multiple times. Also the actual createOpts struct does not rely on the parsed and formatted name but still uses the "raw" driverConfig.Image.

Would you mind to invest a few minutes more to clean it up?

@pabloyoyoista
Copy link
Contributor Author

Absolutely. I'll check out the mentioned PR and clean it up a bit.

Would it maybe make sense to also add a test to catch a possible future regression?

@pabloyoyoista
Copy link
Contributor Author

pabloyoyoista commented Apr 4, 2021

@towe75 should be done now. I have added a test to avoid the regression from happening again. I have tested locally that just running the test without the changes in the first commit would fail with the same result as in the description. However, I though squashing the test and the fix makes more sense than adding a broken commit to the history, even if it makes it harder for verification. Let me know if I should split them.

@towe75
Copy link
Collaborator

towe75 commented Apr 5, 2021

@pablofsf : i found that your PR did not trigger the GH action workflow to run the tests. This problem was not noticed before because most changes still happens through pushes instead of "external" PRs.

Can you please add the pull_request trigger to

within your repo? This should kick off a test-run.

driver.go Show resolved Hide resolved
@towe75
Copy link
Collaborator

towe75 commented Apr 5, 2021

@pablofsf : LGTM, thank you! @drewbailey: do you want to take a look as well?

Copy link
Contributor

@drewbailey drewbailey left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@drewbailey drewbailey merged commit 0ad03f7 into hashicorp:main Apr 5, 2021
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.

4 participants