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

Also check stdout is a tty when requiring interactive credentials #256

Merged
merged 3 commits into from
Sep 30, 2020

Conversation

sbesson
Copy link
Member

@sbesson sbesson commented Sep 24, 2020

Fixes IDR/idr-utils#27

Can be tested with the following minimal workflow

omero logout # ensure no session is active
omero login > out # should fail with an error message
omero login # enter valid credentials
omero login > out # should re-use the existing session

@sbesson
Copy link
Member Author

sbesson commented Sep 29, 2020

As suggested by 606f9fc, one downside of failing in sessions login when stdout is not a tty is tests can no longer simply use pytest capsys fixture to test scenarios where additional input to the CLI control is required. I expect most (if not all) integration tests should be unaffected by this change.

@dominikl
Copy link
Member

That's good, thanks Seb 👍 Often I've been running omero something >> out.log just to discover later that it's been sitting there and waiting for credentials.

Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

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

LGTM. One question about the removed test.

test/unit/clitest/test_sess.py Show resolved Hide resolved
@manics manics merged commit dabb54f into ome:master Sep 30, 2020
@sbesson
Copy link
Member Author

sbesson commented Sep 30, 2020

Should I go ahead and start the process of releasing this as omero-py 5.8.2 or is anything else planned?

@manics
Copy link
Member

manics commented Sep 30, 2020

I'm not aware of anything, maybe check in standup tomorrow?

@sbesson sbesson deleted the stdout_tty branch June 21, 2021 12:27
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.

Stdout not available on stats.py
3 participants