Skip to content

[TF-11813] do not colorize runHeader when returned as err#34473

Merged
laurenolivia merged 1 commit intomainfrom
laurenolivia/fix-cloud-colorize
Jan 17, 2024
Merged

[TF-11813] do not colorize runHeader when returned as err#34473
laurenolivia merged 1 commit intomainfrom
laurenolivia/fix-cloud-colorize

Conversation

@laurenolivia
Copy link
Copy Markdown

@laurenolivia laurenolivia commented Jan 3, 2024

Fixes #

We should not call colorize on strings that are returned as error messages.

Target Release

1.5.x

Draft CHANGELOG entry

NEW FEATURES | UPGRADE NOTES | ENHANCEMENTS | BUG FIXES | EXPERIMENTS

  • Fix terminal error by removing colorized text

Before

Screenshot 2024-01-16 at 3 17 57 PM

After

Screenshot 2024-01-22 at 5 23 15 PM

@laurenolivia laurenolivia changed the title call Colorize() on CLI output [TF-11813] call Colorize() on CLI output Jan 3, 2024
@apparentlymart apparentlymart requested a review from a team January 11, 2024 23:43
@laurenolivia laurenolivia force-pushed the laurenolivia/fix-cloud-colorize branch from 2e10cd7 to a34812c Compare January 16, 2024 20:31
Copy link
Copy Markdown
Contributor

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

❔ 👇

Copy link
Copy Markdown
Contributor

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

There are some peculiar things about this error that I feel compelled to point out that make this bug fix a little less than straightforward.

We are colorizing (that is, replace [color] codes with terminal escape sequences that produce color in a terminal) the text of an error message and returning it as an error type.

The original intent of the error diagnostics are to basically format errors like this:

Screenshot 2024-01-16 at 4 55 25 PM

Specifically, a red bar prefixing a red label "Error:", followed by a white summary, followed by gray details.

By injecting escape codes into the details, we're hacking the error and overriding the intend of the diagnostics output.

If this error were instead used in JSON output, or just logged, or anything other than being displayed in the terminal, the escape characters wouldn't make sense and it would come out like { "error": { "details": "\x1b[1;33mTo view this run in a browser,..." }}

Maybe a better solution is to return an error message that does not "hack" the terminal, which may mean using a string that is not runHeader

@laurenolivia laurenolivia requested a review from brandonc January 17, 2024 21:26
@laurenolivia laurenolivia force-pushed the laurenolivia/fix-cloud-colorize branch from 3ed0e0c to 048818b Compare January 17, 2024 21:42
@laurenolivia laurenolivia changed the title [TF-11813] call Colorize() on CLI output [TF-11813] do not colorize runHeader when returned as err Jan 17, 2024
@laurenolivia laurenolivia force-pushed the laurenolivia/fix-cloud-colorize branch from d28f9b9 to 6d9b3b2 Compare January 17, 2024 22:18
@laurenolivia laurenolivia requested a review from brandonc January 17, 2024 22:27
@laurenolivia laurenolivia merged commit 072dfbd into main Jan 17, 2024
@github-actions
Copy link
Copy Markdown
Contributor

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@github-actions
Copy link
Copy Markdown
Contributor

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 22, 2024
@laurenolivia
Copy link
Copy Markdown
Author

changelog added via #34715

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants