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
Bug 1830080: Prompt user to update traffic on revision deletion #5256
Bug 1830080: Prompt user to update traffic on revision deletion #5256
Conversation
@jeff-phillips-18: This pull request references Bugzilla bug 1830080, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
5587585
to
e8810a0
Compare
@openshift/team-devconsole-ux @serenamarie125 @beaumorley PTAL |
@jeff-phillips-18 Thanks , it looks great. I verified for below scenarios
|
import { RevisionModel } from '../../models'; | ||
import TrafficSplittingFields from '../traffic-splitting/TrafficSplittingFields'; | ||
|
||
export interface TrafficSplittingDeleteModalProps { |
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.
nit: we may not need to export type
import TrafficSplittingFields from '../traffic-splitting/TrafficSplittingFields'; | ||
|
||
export interface TrafficSplittingDeleteModalProps { | ||
revisionItems: any; |
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.
we can type for revisionItems
9c2f9da
to
b2dcb89
Compare
@openshift/team-devconsole-ux @beaumorley |
/test backend |
Hi @jeff-phillips-18 this looks good. For the case were the user has to adjust the split for the two remaining revisions, is it an option to disable the delete button until they equal 100? I don't know if we already have a pattern here to show the error like you do so just asking. Also could we more clearly spell out what they need to do? (A) Make sure you update the traffic distribution among the remaining revisions to equal 100. This might be too conversational but I would want the 100 part at least. So (A) Update the traffic distribution among the remaining revisions to equal 100 <--are there units here? |
@beaumorley Those issues should likely be address in https://issues.redhat.com/browse/ODC-2479. There are more changes to the traffic splitting portion's styling and logic going on there. |
hey @beaumorley ! All good points, we definitely need to improve this dialog in the future - FYI we've talked about making it more consistent with the Kiali dialog, but these things would need to be tackled in future releases. |
b2dcb89
to
e7f2783
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christianvogt, invincibleJai, jeff-phillips-18, serenamarie125 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
@jeff-phillips-18: All pull requests linked via external trackers have merged: openshift/console#5256. Bugzilla bug 1830080 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Fixes:
https://issues.redhat.com/browse/ODC-2413
Analysis / Root cause:
Users are allowed to delete revisions w/o adjusting traffic distribution.
Users are allowed to delete the last revision in a knative service.
Solution Description:
When a revision is deleted, check if it is the only revision in the service, if so prevent the deletions and inform the user.
When a revision is ok to delete, prompt the user to update the traffic distribution prior to deleting the revision.
Screen shots / Gifs for design review:
Single Revision being deleted:
One of three revisions being deleted:
One of two revisions being deleted where the second currently receives no traffic:
Process of deleting a revision:
Browser conformance:
cc @openshift/team-devconsole-ux @serenamarie125 @beaumorley
/kind bug