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

Remove install word in Condition error messages #3044

Merged
merged 2 commits into from
Aug 25, 2023

Conversation

SrinivasAtmakuri
Copy link
Collaborator

Which issue this PR addresses:

With PR: #2450 merged, I have added cluster installation failed in the error message and step hiveClusterDeploymentReady will also be run during the Update path, the existing error message will confuse customers.

This PR modifies the error message by removing the install word in the error.

Fixes

What this PR does / why we need it:

Test plan for issue:

Is there any documentation that needs to be updated for this PR?

@SrinivasAtmakuri SrinivasAtmakuri added size-small Size small ready-for-review skippy pull requests raised by member of Team Skippy labels Jul 19, 2023
@SrinivasAtmakuri SrinivasAtmakuri changed the title Removed install word in Condition error messages Remove install word in Condition error messages Jul 19, 2023
petrkotas
petrkotas previously approved these changes Jul 19, 2023
Copy link
Collaborator

@SudoBrendan SudoBrendan left a comment

Choose a reason for hiding this comment

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

I'll be honest, I'm not intuitively seeing how this wording improves the customer experience. These are customer errors, right? If I'm a customer, I don't care about "conditions" or "hive", etc, what I care about is that I couldn't create a cluster, and ideally, why that happened... These timeouts would be sometimes my problem (e.g. a policy in my sub blocked the deployment, and Tanmay and Swetha are working on changing that), but sometimes our problem as SREs. I'm not sure this new message clarifies any of that to me, from a customer perspective, and actually removes useful information in the message (that the cluster installation has failed).

ArrisLee
ArrisLee previously approved these changes Jul 20, 2023
Copy link
Collaborator

@ArrisLee ArrisLee left a comment

Choose a reason for hiding this comment

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

LGTM

@SrinivasAtmakuri SrinivasAtmakuri dismissed stale reviews from ArrisLee and petrkotas via fc3f09a July 20, 2023 07:25
@SrinivasAtmakuri SrinivasAtmakuri added the next-release To be included in the next RP release rollout label Jul 20, 2023
@rogbas
Copy link
Collaborator

rogbas commented Jul 20, 2023

LGTM but I don't think we need to rush, cut a new release and re-run INT to include this change.

IMO it could wait for next release

@petrkotas
Copy link
Member

@rogbas I greee, it is LGTM, it makes sense to me, but it can wait.

Copy link
Collaborator

@UlrichSchlueter UlrichSchlueter left a comment

Choose a reason for hiding this comment

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

Not sure the error messages are actually all actionable. Might be a good idea to discuss this with the PMs for a next iteration.

@cadenmarchese cadenmarchese merged commit 644f8b8 into Azure:master Aug 25, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-release To be included in the next RP release rollout ready-for-review size-small Size small skippy pull requests raised by member of Team Skippy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants