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

DM -20325: Add interactive option #44

Merged
merged 1 commit into from Jun 28, 2019
Merged

DM -20325: Add interactive option #44

merged 1 commit into from Jun 28, 2019

Conversation

SimonKrughoff
Copy link
Contributor

This will keep passwords secret when running from the command line.

{Summary of changes. Prefix PR title with ticket handle.}

  • Passes Jenkins CI.
  • Documentation is up-to-date.

Preview the docs at: https://pipelines.lsst.io/v/DM-FIXME

@SimonKrughoff SimonKrughoff removed the request for review from jonathansick June 27, 2019 19:19
@SimonKrughoff
Copy link
Contributor Author

Turns out, I'm not ready for review yet.

@SimonKrughoff
Copy link
Contributor Author

OK. Now ready. I'd be open to suggestions on how to test the command line tool.

jonathansick
jonathansick reviewed Jun 27, 2019
View reviewed changes
Copy link
Member

@jonathansick jonathansick left a comment

Choose a reason for hiding this comment

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

What do you think about automatically falling back to the prompt if the other ways of getting a password in aren't used?

python/lsst/verify/bin/dispatchverify.py Outdated
@@ -128,6 +129,12 @@ def parse_args():
default=False,
help='Ignore data blobs even if they are available in the verification'
'job.')
parser.add_argument(
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend dropping the flag and if neither the args.api_password is set nor is the SQUASH_password set, then automatically fallback to prompting for the password.

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 was going to do that, but then I realized that in non-interactive situations it will hang forever if neither is set. That seemed like a bad failure mode.

My other thought was to fall back to prompting, but put a timeout on the input.

Copy link
Member

Choose a reason for hiding this comment

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

I was going to do that, but then I realized that in non-interactive situations it will hang forever if neither is set. That seemed like a bad failure mode.

I think that's fine, to be honest. Most jobs will timeout on their own if it actually happens.

I can't think of any command line program that has a command line option to open an interactive prompt rather than doing it automatically if necessary, so this --interactive option seems to run against convention.

python/lsst/verify/bin/dispatchverify.py
self.api_password = getpass.getpass(prompt="SQuaSH password: ")
else:
self.api_password = (args.api_password or
os.getenv('SQUASH_password'))
if not self.test and self.api_password is None:
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 recall what self.test is, but I think you'd want to make the logic for opening a prompt also be bypass-able through the self.test flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean bypass if --interactive is set and test is set?

Copy link
Member

Choose a reason for hiding this comment

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

If the --test flag was set by a user, don't use getpass.getpass to request a password since that password won't be used.

jonathansick
jonathansick reviewed Jun 27, 2019
View reviewed changes
Copy link
Member

@jonathansick jonathansick left a comment

Choose a reason for hiding this comment

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

Also, you might want to update the help string for the --password option to say something along the lines that if neither --password nor $SQUASH_password are set, the program will prompt you for a password.

@SimonKrughoff
Copy link
Contributor Author

@jonathansick is this more to your liking?

jonathansick
jonathansick approved these changes Jun 28, 2019
View reviewed changes
Copy link
Member

@jonathansick jonathansick left a comment

Choose a reason for hiding this comment

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

Perfect thanks. Just one more idea about the help message :)

python/lsst/verify/bin/dispatchverify.py Outdated
@@ -182,7 +183,7 @@ def parse_args():
dest='api_password',
metavar='PASSWORD',
help='Password for SQUASH API. Equivalent to the $SQUASH_PASSWORD '
'environment variable.')
'environment variable. If neither is set, user will be prompted.')
Copy link
Member

Choose a reason for hiding this comment

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

user -> you

@SimonKrughoff
Prompt user for password if not supplied
c2d9751 
This will keep passwords secret when running from the command line.
@SimonKrughoff SimonKrughoff merged commit c2d9751 into master Jun 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathansick jonathansick jonathansick approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@SimonKrughoff @jonathansick