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.

Sign up for GitHub

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

Jump to bottom

fix bug check nonexist authfile #4337

Merged
merged 1 commit into from
Nov 8, 2019
Merged

fix bug check nonexist authfile #4337

merged 1 commit into from
Nov 8, 2019

Conversation

QiWang19
Copy link
Contributor

@QiWang19 QiWang19 commented Oct 24, 2019 •
edited
Loading

For podman command(except login), if authfile does not exist returns error.

close #4328
Signed-off-by: Qi Wang qiwan@redhat.com

@QiWang19
Copy link
Contributor Author

@TomSweeneyRedHat @rhatdan PTAL

TomSweeneyRedHat
TomSweeneyRedHat reviewed Oct 24, 2019
View reviewed changes
Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

A couple of nits, otherwise LGTM. Nicely done @QiWang19

@mheon
Copy link
Member

mheon commented Oct 24, 2019

/approve
Change LGTM, but CI is broken

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon, QiWang19

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2019
@QiWang19 QiWang19 force-pushed the check_auth_path branch 3 times, most recently from cce152e to a8fb99d Compare October 24, 2019 16:45
TomSweeneyRedHat
TomSweeneyRedHat reviewed Oct 24, 2019
View reviewed changes
test/e2e/search_test.go
search := podmanTest.Podman([]string{"search", "--authfile", "/tmp/nonexist", "alpine"})
search.WaitWithDefaultTimeout()
Expect(search.ExitCode()).To(Not(Equal(0)))
})
Copy link
Member

Choose a reason for hiding this comment

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

can you add tests for the push, run and play-kube commands? Are any changes needed for cmd/podman/run.go or cmd/podman/play_kube.go too? I would suspect so.

Copy link
Member

Choose a reason for hiding this comment

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

@QiWang19 can you answer this one please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tests were added

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a change to test/e2e/run_test.go, test/e2e/push_test.go or test/e2e/play_kube_test.go in this pr....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

push tests are in login_logout_test.go. I will add tests to run_test.go

Copy link
Member

Choose a reason for hiding this comment

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

👍 thx, I'd rather have an additional test in push_test.go, but I won't hold this up further for that.

@rhatdan
Copy link
Member

rhatdan commented Oct 24, 2019

Can't we just do the shared.GetAuthFile() in the options handling, so we check if the environment variable is set and then we don't need to change all this code?

@QiWang19 QiWang19 force-pushed the check_auth_path branch 3 times, most recently from dd4a772 to 85f34b6 Compare October 24, 2019 19:31
@QiWang19
Copy link
Contributor Author

this will fail if I don't have default /run/user/1000/containers/auth.json, should we consider this situation that ? I think CI fails for this reason

podman search alpine
ERRO[0000] error checking authfile path /run/user/1000/containers/auth.json: stat /run/user/1000/containers/auth.json: no such file or directory

TomSweeneyRedHat
TomSweeneyRedHat reviewed Oct 24, 2019
View reviewed changes
cmd/podman/build.go Outdated
@@ -155,6 +155,10 @@ func buildCmd(c *cliconfig.BuildValues) error {
}
}

if _, err := os.Stat(c.BudResults.Authfile); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I think the problem with the test is you first need to check if the Authfile has been set. This gets a little wonky due to the Cobra includes, but I think you can check that with c.Flag("authfile").Changed. You'll need to play/test that making sure that gets tripped appropriately as you pass the auth-file option into the build command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this c.Flag("authfile").Changedonly works for those check that directory runs os.stats under /cmd.
maybe I should get rid of shared.GetSystemContext, just c.Flag("authfile").Changedand os.stats for each cmd

@rhatdan
Copy link
Member

rhatdan commented Oct 25, 2019

I would like to remove the /run/user... stuff from defaults and allow containers/image to handle it, especially if we move to Keyrings as the default, Then we can rely on C/I to specify the default localtion.

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #4310) made this pull request unmergeable. Please resolve the merge conflicts.

@QiWang19 QiWang19 force-pushed the check_auth_path branch 2 times, most recently from b8ddd27 to 0eedbd9 Compare November 1, 2019 19:46
@QiWang19 QiWang19 force-pushed the check_auth_path branch 6 times, most recently from 7fc1b8f to 29d543a Compare November 5, 2019 18:26
@rhatdan
Copy link
Member

rhatdan commented Nov 5, 2019

LGTM

@rhatdan
Copy link
Member

rhatdan commented Nov 5, 2019

@TomSweeneyRedHat @mheon @giuseppe @vrothberg @baude @jwhonce PTAL

TomSweeneyRedHat
TomSweeneyRedHat reviewed Nov 5, 2019
View reviewed changes
cmd/podman/runlabel.go
return errors.Wrapf(err, "error getting authfile %s", c.Authfile)
}
}

Copy link
Member

Choose a reason for hiding this comment

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

We seem to be doing this in many of the commands. Should we make a little utility function in cmd/podman/utils.go?

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 think I should add this kind of function in buildah and vendor from there. And add check nonexist authfile in buildah, too.

Copy link
Member

Choose a reason for hiding this comment

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

That's fine by me if that works better.

test/e2e/search_test.go
search := podmanTest.Podman([]string{"search", "--authfile", "/tmp/nonexist", "alpine"})
search.WaitWithDefaultTimeout()
Expect(search.ExitCode()).To(Not(Equal(0)))
})
Copy link
Member

Choose a reason for hiding this comment

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

@QiWang19 can you answer this one please?

@QiWang19
fix bug check nonexist authfile
d7c0f96 
Use GetDefaultAuthFile() from buildah.
For podman command(except login), if authfile does not exist returns error.

close containers#4328

Signed-off-by: Qi Wang <qiwan@redhat.com>
@TomSweeneyRedHat
Copy link
Member

LGTM
Thanks for sticking at it on this PR @QiWang19 !

@TomSweeneyRedHat
Copy link
Member

and green test buttons if someone wants to LGTM it and merge.

@QiWang19
Copy link
Contributor Author

QiWang19 commented Nov 6, 2019

and green test buttons if someone wants to LGTM it and merge.

merged this PR after buildah PR containers/buildah#1967 got merged so I can vendor in?

@TomSweeneyRedHat
Copy link
Member

@QiWang19 I'm a bit confused by your last comment. Do you want to merge this after containers/buildah#1967 gets merged? Then you want to vendor Buildah into Podman once that's in?

@QiWang19
Copy link
Contributor Author

QiWang19 commented Nov 6, 2019

@TomSweeneyRedHat yes #4337 (comment) if I should add this utility function. That function will be used both in buildah and podman.

@rhatdan
Copy link
Member

rhatdan commented Nov 8, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2019
@openshift-merge-robot openshift-merge-robot merged commit f456ce9 into containers:master Nov 8, 2019
@QiWang19 QiWang19 deleted the check_auth_path branch June 26, 2020 15:11
This was referenced Aug 8, 2020
Open
Open
@Luap99 Luap99 mentioned this pull request May 2, 2023
Closed
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@jwhonce jwhonce Awaiting requested review from jwhonce

@rhatdan rhatdan Awaiting requested review from rhatdan

Assignees

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Podman doesn't check existance of given --authfile
7 participants
@QiWang19 @mheon @openshift-ci-robot @rhatdan @rh-atomic-bot @TomSweeneyRedHat @openshift-merge-robot