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

cli: do not prompt when there is no tty available on stdin #2570

Merged
merged 2 commits into from
Jun 6, 2019

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented May 22, 2019

This is a bit of a drive-by. I noticed that inside travis
snapcraft 3 hangs with the following message:

Support for 'LXD' needs to be set up. Would you like to do that it now? [y/N]:

It seems like we should only prompt here if stdin is attached
to a tty and otherwise just fail early. This should fix hangs
in e.g. scripts.

Copy link
Contributor

@zyga zyga left a comment

Choose a reason for hiding this comment

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

Looks good to me :-)

This is a bit of a drive-by. I noticed that inside travis
snapcraft 3 hangs with the following message:
```
Support for 'LXD' needs to be set up. Would you like to do that it now? [y/N]:
```
It seems like we should only prompt here if stdin is attached
to a tty and otherwise just fail early. This should fix hangs
in e.g. scripts.
@sergiusens
Copy link
Collaborator

Interesting, last I read click.confirm was supposed to take this into account. Mind if I take this further and centralize this change considering that we have https://github.com/snapcore/snapcraft/blob/master/snapcraft/internal/indicators.py#L107?

@mvo5
Copy link
Contributor Author

mvo5 commented May 23, 2019

@sergiusens When I played around with redirection it seems like click.confirm() does not take tty into account. But caveats apply, I did not look deeply and did not report an upstream bug (probably a good idea though). Go wild on this, this PR is just a drive-by as I was slightly annoyed by this hang in travis.

Copy link
Collaborator

@sergiusens sergiusens left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, we will look into consolidating these scenarios into a follow-up PR

@codecov-io
Copy link

codecov-io commented Jun 6, 2019

Codecov Report

Merging #2570 into master will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2570      +/-   ##
==========================================
+ Coverage   89.05%   89.06%   +<.01%     
==========================================
  Files         205      205              
  Lines       13987    13988       +1     
  Branches     2118     2118              
==========================================
+ Hits        12456    12458       +2     
+ Misses       1081     1080       -1     
  Partials      450      450
Impacted Files Coverage Δ
snapcraft/cli/lifecycle.py 86.95% <50%> (+0.09%) ⬆️
snapcraft/internal/elf.py 80.71% <0%> (-0.36%) ⬇️
snapcraft/internal/pluginhandler/_plugin_loader.py 85.29% <0%> (+1.96%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d661c7...b5a90b4. Read the comment docs.

@sergiusens sergiusens merged commit 51b5eb8 into canonical:master Jun 6, 2019
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.

4 participants