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.

Sign up for GitHub

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

Jump to bottom

Generalize default webhook message #50696

Merged
merged 3 commits into from May 2, 2024
Merged

Generalize default webhook message #50696

merged 3 commits into from May 2, 2024

Conversation

leosarra
Copy link
Contributor

@leosarra leosarra commented Apr 26, 2024 •
edited by istio-policy-bot

Please provide a description of this PR:

Fixes #50687

@leosarra leosarra requested a review from a team as a code owner April 26, 2024 13:14
@istio-policy-bot istio-policy-bot added area/environments area/user experience release-notes-none Indicates a PR that does not require release notes. labels Apr 26, 2024
@istio-testing istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 26, 2024
@leosarra
Copy link
Contributor Author

@craigbox

howardjohn
howardjohn reviewed Apr 26, 2024
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

It still impacts ambient. Gateways, for example, are injected.

There is also leader election, etc impacted. Probably even the "before" state was not great.

I am not sure a good way to word it though 🙂

linsun
linsun reviewed Apr 29, 2024
View reviewed changes
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 also share the concern of @leosarra and wanted to change the msg every time I install istio :)

How about just printing out:
if ambient:
The ambient profile has been installed successfully, enjoy Istio without sidecars!
else:
same msg as today?

@howardjohn
Copy link
Member

We could also generally say "Set as the default for cluster-wide operations"

@craigbox
Copy link
Contributor

The ambient profile has been installed successfully, enjoy Istio without sidecars!

You're really leaning into the cutesy lately there Lin :)

@linsun
Copy link
Member

linsun commented Apr 30, 2024

The ambient profile has been installed successfully, enjoy Istio without sidecars!

You're really leaning into the cutesy lately there Lin :)

Mainly to emphasize the message without sidecars :)

@leosarra leosarra requested a review from a team as a code owner May 2, 2024 13:49
@leosarra
Copy link
Contributor Author

leosarra commented May 2, 2024

Sorry for the delay, I had some days off.

We could also generally say "Set as the default for cluster-wide operations"

I like the idea of making the msg more general. Updated to "Made this installation the default for cluster-wide operations.".

I have also added a post-install msg specific to ambient based on @linsun proposal.

@leosarra leosarra changed the title Use different webhook msg for ambient mode Generalize default webhook message May 2, 2024
@istio-testing istio-testing merged commit 96a6174 into istio:master May 2, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howardjohn howardjohn howardjohn left review comments

@linsun linsun linsun approved these changes

Assignees
No one assigned
Labels
area/environments area/user experience release-notes-none Indicates a PR that does not require release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

don't talk about injection if the install profile is ambient
6 participants
@leosarra @howardjohn @craigbox @linsun @istio-testing @istio-policy-bot