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

Fixes #28281 - no spinner on non-tty output #291

Merged
merged 1 commit into from
May 11, 2020

Conversation

lzap
Copy link
Member

@lzap lzap commented Nov 18, 2019

When the tool is executed with redirected output (to a pipe or a file) the spinner thing makes thing terrible and unreadable. The process should detect if it's running a tty and then avoid spinning (e.g. when executed from cron).

Reproducer:

foreman-maintain service status > /tmp/test.log

See the ^M characeters in the log file. If the action takes a lot of time (e.g. restart) there will be a lots of these.

@theforeman-bot
Copy link
Member

Issues: #28281

@mbacovsky
Copy link
Member

@lzap thanks for the patch. LGTM!
@jameerpathan111, could you please confirm the suggested change does not have any unwanted effect in our automation?

@jameerpathan111
Copy link
Contributor

@lzap thanks for the patch. LGTM!
@jameerpathan111, could you please confirm the suggested change does not have any unwanted effect in our automation?

Sorry for late reply, I will look into this.

@jameerpathan111
Copy link
Contributor

@mbacovsky @lzap I can still see the ^M characeters in the test.log file
I have run foreman-maintain service status > /tmp/test.log.

@lzap
Copy link
Member Author

lzap commented Dec 3, 2019

This patch does not remove all of them, just those which were generated via the spinner. So many of them. Can you paste the output tho?

@kgaikwad
Copy link
Member

This patch does not remove all of them, just those which were generated via the spinner. So many of them. Can you paste the output tho?
@jameerpathan111, could you please share the output here?

@jameerpathan111
Copy link
Contributor

@kgaikwad
Attaching the output of foreman-maintain service status > /tmp/test.log using this PR.
fm_pr_291.log

@lzap
Copy link
Member Author

lzap commented May 11, 2020

The text looks quite readable now, I mean there are few leftovers but it's not polluted with ^M^M^M^M.

I don't see a reason why not to merge it, unless it breaks something in your tests.

@jameerpathan111
Copy link
Contributor

The text looks quite readable now, I mean there are few leftovers but it's not polluted with ^M^M^M^M.
I don't see a reason why not to merge it, unless it breaks something in your tests.

yeah, it doesn't break anything. We can ignore few of ^M characters.

@kgaikwad
Copy link
Member

Sorry for the delay @lzap. Merging this patch.

@kgaikwad kgaikwad merged commit a384ab1 into theforeman:master May 11, 2020
@kgaikwad
Copy link
Member

Thank you @lzap and @jameerpathan111.

@jameerpathan111
Copy link
Contributor

jameerpathan111 commented May 12, 2020

Note: @lzap @kgaikwad with and without pr change, it is showing almost same ^M chars. Just few of them are removed. :(

@kgaikwad
Copy link
Member

@jameerpathan111 - As long as no any unwanted failure in automation or tests, I guess that's fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants