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

fix: don't render spinner in non-TTY terminal #13242

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

sobolk
Copy link
Member

@sobolk sobolk commented Sep 19, 2023

Description of changes

This PR makes Spinner noop in non-TTY terminals.

Customers who spawn CLI process as child process or stream logs to files observe control characters and transitions in output. These are not needed in these scenarios and only obscure output.

Issue #, if available

Description of how you validated changes

  1. Added unit tests
  2. Ran full e2e suite.
  3. Manual checks
    1. Stream to logs withouth fix
    2. Stream to logs with fix
    3. push in TTY terminal to validate spinner is rendering

Before change - log to file
image

After change - log to file
image

TTY
(see spinner next to "This will take few minutes", the other is harder to catch as it uploads faster).
image

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
  • Pull request labels are added

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sobolk sobolk marked this pull request as ready for review September 19, 2023 20:23
@sobolk sobolk requested a review from a team as a code owner September 19, 2023 20:23
@sobolk sobolk merged commit d75d0b2 into aws-amplify:dev Sep 19, 2023
@sobolk sobolk deleted the spinner-tty branch September 19, 2023 22:22
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.

3 participants