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

add fancier success icons #50744

Merged
merged 4 commits into from May 14, 2024
Merged

Conversation

ilrudie
Copy link
Contributor

@ilrudie ilrudie commented Apr 29, 2024

Please provide a description of this PR:

added fancier icons for success statuses in istioctl

@ilrudie ilrudie requested a review from a team as a code owner April 29, 2024 21:33
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 29, 2024
Signed-off-by: Ian Rudie <[email protected]>
Signed-off-by: Ian Rudie <[email protected]>
@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 29, 2024
Signed-off-by: Ian Rudie <[email protected]>
Comment on lines 94 to 95
IngressComponentName: "🚪",
EgressComponentName: "✋",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use similar or same icons for ingress and egress?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about 🛫 (plane takeoff/leave) for egress and 🛬 (plane land/arrive) for ingress.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@linsun, how do you feel about using the planes?

var IstioComponentSuccessIcons = map[ComponentName]string{
IstioBaseComponentName: "⛵️",
PilotComponentName: "🧠",
CNIComponentName: "🚦",
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it makes sense to use the CNI icon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally that'd be great but I don't think there is a unicode character for it. I'll take another look though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried 🪢 which sort of looks like ethernet cables being woven

Copy link
Member

@linsun linsun left a comment

Choose a reason for hiding this comment

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

I like the istio base, pilot and ztunnel ones, nice!!!

@linsun linsun added the release-notes-none Indicates a PR that does not require release notes. label May 14, 2024
Copy link
Member

@linsun linsun left a comment

Choose a reason for hiding this comment

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

LGTM

Sending to ambient-dev to see if others have comments.

Copy link
Contributor

@MorrisLaw MorrisLaw left a comment

Choose a reason for hiding this comment

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

This lgtm, thanks Ian!

@linsun
Copy link
Member

linsun commented May 14, 2024

/test release-notes

@istio-testing istio-testing merged commit b593318 into istio:master May 14, 2024
28 checks passed
@ilrudie ilrudie deleted the fancy-status-icons branch May 14, 2024 20:06
@stevenctl
Copy link
Contributor

Most of these characters don't render for me. I think seeing squares wouldn't be the best look. Not everyones terminal/font will have full unicode/emoji support.

⛵️🧠🪢🔒🛬🛫✅❌✅❌

Only 3 of these (the sailboat, the knot and the lock) render for me.

@hanxiaop
Copy link
Member

In my case, the first letter of the last message is cut off:
image

@linsun
Copy link
Member

linsun commented May 15, 2024

Good it is in master :) thanks @stevenctl and @hanxiaop for reporting it. cc @ilrudie

@ilrudie
Copy link
Contributor Author

ilrudie commented May 15, 2024

We can revert it if folks want and I can look at some detection methods so we can have like a fancy set and a safe set to choose from based on detecting what is/isn't supported

@ilrudie
Copy link
Contributor Author

ilrudie commented May 15, 2024

An alternative might be to use a simple up/down to indicate success as we were previously but decorate the end of the line with fancy characters. this way if we do get the mystery char box at least what occurred is clear and still readable.

when supported:

✔ Istio core installed ⛵️

or when not supported:

✔ Istio core installed �

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes-none Indicates a PR that does not require release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants