fix mandatory parameter in login/logout #5233
Merged
Conversation
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
cmd/podman/logout.go
Outdated
| return errors.Errorf("registry must be given") | ||
| registriesFromFile, err := registries.GetRegistries() | ||
| if err != nil || len(registriesFromFile) == 0 { | ||
| return errors.Errorf("registry must be given") |
There was a problem hiding this comment.
suggest:
"no registries found in registries.conf, a registry must be provided"
QiWang19
force-pushed
the
login/out-parameter
branch
from
0b26f48
to
8923e82
Compare
|
LGTM |
|
@QiWang19 looks like you've some test unhappiness that needs to be tended to. |
|
@QiWang19 Please fix the tests so we can get this merged. |
QiWang19
force-pushed
the
login/out-parameter
branch
from
8923e82
to
f81319a
Compare
|
@rhatdan @TomSweeneyRedHat we can get this in |
test/e2e/login_logout_test.go
Outdated
| session := podmanTest.Podman([]string{"login", "-u", "podmantest", "-p", "test"}) | ||
| session.WaitWithDefaultTimeout() | ||
| Expect(session.LineInOutputContains("docker.io")) | ||
| Expect(session.ExitCode()).To(Not(Equal(0))) |
There was a problem hiding this comment.
I think this is testing the case when there's no registry in registry.conf and it fails. Can we test the case when there is an entry in registries.conf that woud succeed?
QiWang19
force-pushed
the
login/out-parameter
branch
from
f81319a
to
4ea7e6f
Compare
|
/approve |
openshift-ci-robot
added
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: QiWang19, rhatdan 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 |
openshift-ci-robot
added
the
approved
fix containers#5146 Insted of using a registry as mandatory parameter, this path allows podman to use the first registry from registries.conf. Signed-off-by: Qi Wang <[email protected]>
QiWang19
force-pushed
the
login/out-parameter
branch
from
4ea7e6f
to
4c13501
Compare
|
/lgtm |
|
LGTM |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/test images |
edsantiago
added a commit
to edsantiago/libpod
that referenced
this pull request
Apr 29, 2020
If a usage message is of the form '... [flags] ARGNAME', where ARGNAME is all-caps and not in brackets, it must be a required argument. Try running podman subcommand without ARGNAME, and make sure that podman bails out with an informative message. (Since this message is freeform in each subcommand, not Cobra-generated, we have a lot of possible variations to check for). Fix podman login/logout Use messages to indicate that REGISTRY is now optional (as of containers#5233). This test has actually been in place for over a year but due to a typo on my part -- a missing space -- it was not being run. "For want of a space, much testing was lost". Signed-off-by: Ed Santiago <[email protected]>
baude
pushed a commit
to baude/podman
that referenced
this pull request
Apr 30, 2020
If a usage message is of the form '... [flags] ARGNAME', where ARGNAME is all-caps and not in brackets, it must be a required argument. Try running podman subcommand without ARGNAME, and make sure that podman bails out with an informative message. (Since this message is freeform in each subcommand, not Cobra-generated, we have a lot of possible variations to check for). Fix podman login/logout Use messages to indicate that REGISTRY is now optional (as of containers#5233). This test has actually been in place for over a year but due to a typo on my part -- a missing space -- it was not being run. "For want of a space, much testing was lost". Signed-off-by: Ed Santiago <[email protected]>
snj33v
pushed a commit
to snj33v/libpod
that referenced
this pull request
May 31, 2020
If a usage message is of the form '... [flags] ARGNAME', where ARGNAME is all-caps and not in brackets, it must be a required argument. Try running podman subcommand without ARGNAME, and make sure that podman bails out with an informative message. (Since this message is freeform in each subcommand, not Cobra-generated, we have a lot of possible variations to check for). Fix podman login/logout Use messages to indicate that REGISTRY is now optional (as of containers#5233). This test has actually been in place for over a year but due to a typo on my part -- a missing space -- it was not being run. "For want of a space, much testing was lost". Signed-off-by: Ed Santiago <[email protected]>
github-actions
bot
added
the
locked - please file new issue/PR
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
fix #5146
Insted of using a registry as mandatory parameter, this path allows podman to use the first registry from registries.conf.
Signed-off-by: Qi Wang [email protected]