-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: show tier-releated error msg for HVD clusters upon sync activation #27189
Conversation
CI Results: |
if (this.args.isHvdManaged) { | ||
errors.push( | ||
'Secrets Sync is available for Plus tier clusters only. Please check the tier of your cluster to enable Secrets Sync.' | ||
); | ||
} | ||
this.activationErrors = errors; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is subtle, but it ensures we display both the raw API error and a more HVD-specific error in case the reason for the failure is due to the cluster having an incorrect tier. the result is 2 error banners, like so:
i considered showing only the tier error, but i didn't feel good about obfuscating the true server error just in case it occurred for any other reason (even though it should be very unlikely). i also considered trying to keep both error messages within the same error banner but that would require edits to the <MessageError />
component which started to feel like too much scope creep for an item we'd like to get in ASAP.
open to other ideas!
a0c1474
to
535d15b
Compare
Build Results: |
@action | ||
clearActivationErrors() { | ||
this.activationErrors = null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, looks like i missed this when implementing the sync activation modal. this ensures those errors are cleared once a user re-clicks the "confirm" button in the activation modal.
@onDismiss={{fn (mut this.hideError) true}} | ||
data-test-opt-in-error | ||
/> | ||
<MessageError @errors={{this.activationErrors}} @onDismiss={{fn (mut this.hideError) true}} data-test-opt-in-error /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the dismiss "x" on the upper right of the error banners, does this mean if I click one dismiss it will dismiss both banners?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it sure does. that's a great point. that's a bug with the <MessageErrors />
component implementation of using its @onDismiss
args -- it applies the same callback to every alert rendered:
vault/ui/lib/core/addon/templates/components/message-error.hbs
Lines 7 to 12 in a953a3c
{{#each this.displayErrors as |error|}} | |
<Hds::Alert | |
data-test-message-error | |
@type="inline" | |
@color="critical" | |
@onDismiss={{@onDismiss}} |
i'll file a bug report for this. great find!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. One question in the hbs file and then a reminder to run enterprise test locally yarn run test:filter -f=enterprise
. You'll see on recent PRs that we're indicating these were run with a checkbox on the pr description. We only need to run them if the code touches enterprise features, which this does.
π οΈ Description
β show HVD clusters a more specific error when attempts to activate fail
π π§Ή ensure activation errors are cleared once the user tries to re-activate
πΈ Screenshots
Screen.Recording.2024-05-22.at.5.25.42.PM.mov
ποΈ How to Build and Test the Change
The best way to reproduce this is by hard coding the
flags
service soisHvdManaged
returnstrue
, and then forcing an error to the activate endpoint by changing the API path to something invalid. Check out the screenshot below for an example: