Skip to content

Warn user about --password cli option in login #5268

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
Merged
openshift-merge-robot merged 1 commit into from
Feb 20, 2020
Merged

Warn user about --password cli option in login #5268

openshift-merge-robot merged 1 commit into from
Feb 20, 2020

Conversation

@Akasurde
Copy link
Contributor

@Akasurde Akasurde commented Feb 19, 2020

Using command line parameter --password may reveal registry password. Better to warn user about this.

Signed-off-by: Abhijeet Kasurde [email protected]

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Feb 19, 2020

Hi @Akasurde. Thanks for your PR.

I'm waiting for a containers member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS labels Feb 19, 2020
@Akasurde
Copy link
Contributor Author

Akasurde commented Feb 19, 2020

/assign @umohnani8

@mheon
Copy link
Member

mheon commented Feb 19, 2020

/approve
/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 19, 2020
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Feb 19, 2020

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details 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 Feb 19, 2020
mheon
mheon reviewed Feb 19, 2020
View reviewed changes
@Akasurde Akasurde requested review from mheon and rhatdan February 20, 2020 09:58
@rhatdan
Copy link
Member

rhatdan commented Feb 20, 2020

LGTM

@mheon
Copy link
Member

mheon commented Feb 20, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2020
@openshift-merge-robot openshift-merge-robot merged commit e7d8bda into containers:master Feb 20, 2020
@Akasurde Akasurde deleted the warn_bare_password branch February 20, 2020 14:58
@Akasurde
Copy link
Contributor Author

Akasurde commented Feb 20, 2020

@mheon @rhatdan Thanks for the review and merge.

@edsantiago edsantiago mentioned this pull request Feb 20, 2020
Merged
edsantiago added a commit to edsantiago/libpod that referenced this pull request Feb 20, 2020
@edsantiago
Login test: use --password-stdin
9ecf8e1 
Great timing: this new test collided against containers#5268, which added
a warning about using command-line --password. CI is now going
to fail all over.

Fix: rework test to use --password-stdin. Am doing so only
in the places where output string is checked; other instances
can keep using '--password xxx' because it's simpler.

Signed-off-by: Ed Santiago <[email protected]>
@edsantiago edsantiago mentioned this pull request Feb 20, 2020
Merged
@edsantiago
Copy link
Member

edsantiago commented Feb 20, 2020

Day late dollar short, but would you consider opening a new PR using logrus.Warn() instead of printf?

@mheon
Copy link
Member

mheon commented Feb 20, 2020

Warnf isn't printed by default, which could be a problem

@edsantiago
Copy link
Member

edsantiago commented Feb 20, 2020

Warnf isn't printed by default

:gaping-mouth-agog-emoji: I didn't realize that. Um, ok, never mind. Thanks for the quick response.

@mheon
Copy link
Member

mheon commented Feb 20, 2020

I kind of want to change that, but we have a bunch of warnings that will show by default now. We'd need to do a pass over all our logging and determine what severity things should be (and then try and fix any warnings that were still being displayed)

@rhatdan
Copy link
Member

rhatdan commented Feb 21, 2020

I agree, but I believe these warnings run deep.

snj33v pushed a commit to snj33v/libpod that referenced this pull request May 31, 2020
@edsantiago @snj33v
Login test: use --password-stdin
b7f434a 
Great timing: this new test collided against containers#5268, which added
a warning about using command-line --password. CI is now going
to fail all over.

Fix: rework test to use --password-stdin. Am doing so only
in the places where output string is checked; other instances
can keep using '--password xxx' because it's simpler.

Signed-off-by: Ed Santiago <[email protected]>
@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 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@TomSweeneyRedHat TomSweeneyRedHat Awaiting requested review from TomSweeneyRedHat

@vrothberg vrothberg Awaiting requested review from vrothberg

@rhatdan rhatdan Awaiting requested review from rhatdan

@mheon mheon Awaiting requested review from mheon

Assignees

@mheon mheon

@umohnani8 umohnani8

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. ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Akasurde @openshift-ci-robot @mheon @rhatdan @edsantiago @umohnani8 @openshift-merge-robot