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

mgr/dashboard: Cleanup: Editing the order of the properties in Create/Edit Silence form #34511

Closed

Conversation

nizamial09-zz
Copy link

@nizamial09-zz nizamial09-zz commented Apr 10, 2020

This commit is intended as a cleanup for the create/edit silence form which rearranges the form elements.

Screenshot from 2020-04-10 19-44-01

Fixes: https://tracker.ceph.com/issues/42305

Signed-off-by: Nizamudeen nia@redhat.com

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard backend
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

…/Edit Silence form

This commit is intended as a cleanup for the create/edit silence form which rearranges the form elements.

Fixes: https://tracker.ceph.com/issues/42305

Signed-off-by: Nizamudeen <nia@redhat.com>
Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

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

👍 thanks @nizamial09 !

@LenzGr LenzGr requested a review from Devp00l April 14, 2020 12:52
@nizamial09-zz nizamial09-zz changed the title mgr/dashboard: Cleanup: Editing the order of the properties in Create… mgr/dashboard: Cleanup: Editing the order of the properties in Create/Edit Silence form Apr 16, 2020
Copy link

@Devp00l Devp00l left a comment

Choose a reason for hiding this comment

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

Sorry I haven't seen the issue before - I have my doubts if we should include the change as it would be a regression IMO. (tracker comment)

@LenzGr
Copy link
Contributor

LenzGr commented Apr 17, 2020

Hmm, I admit I am torn about the appropriate order of these elements. Reviewing the tracker issue and Stephan's comment, I'd inclined to reject this PR, as I too think that the current order feels more "natural": as an admin I have a specific reason for silencing an alert, so adding this in a comment should come first (the "Why?"), followed by the start time and duration/end time (the "When?"). It remains to be debated if the matchers section (the "What?") should actually be put before the comment and time definitions, though, so the order would be:

  • What?
  • Why?
  • When?

Thoughts?

@nizamial09-zz
Copy link
Author

Hmm, I admit I am torn about the appropriate order of these elements. Reviewing the tracker issue and Stephan's comment, I'd inclined to reject this PR, as I too think that the current order feels more "natural": as an admin I have a specific reason for silencing an alert, so adding this in a comment should come first (the "Why?"), followed by the start time and duration/end time (the "When?"). It remains to be debated if the matchers section (the "What?") should actually be put before the comment and time definitions, though, so the order would be:

  • What?
  • Why?
  • When?

Thoughts?

This order makes sense to me. I am not an expert on this but as far as I can see, It would be sensible to specify the start time and duration/end time only after selecting the matchers. And also the comment section coming before the time definitions are also a better option. Surely this needs some discussion. @LenzGr @Devp00l

@nizamial09-zz
Copy link
Author

While we are at this, it would be great to have a little discussion area for this reported bug as well:
https://tracker.ceph.com/issues/42308

@Devp00l
Copy link

Devp00l commented Apr 20, 2020

@LenzGr The comment should describe in words why the silence is in place, what matchers it has is irrelevant for the viewer in of the table (which only sees comment, creator, start and end time), as it can be seen as an implementation detail. When viewing the list we should assume that the right matchers are in place to tackle the problem in time. Same like crush profiles for pools, we should assume they work out for the pool in the table otherwise we will know through failures and delete the pool and the rule (which is hopefully not the case).

IMO the current order is straight forward as you can add multiple silence but you can only set the start and end time once - this means the form will not expand downwards on the adding of a matcher and it has the same order as shown in the table view.

@nizamial09 Added a comment to the other issue and changed it into a feature as it's not a bug.

@nizamial09-zz
Copy link
Author

Before closing this PR out there are a series of cleanup issues related to this tracker issue which can be seen on https://tracker.ceph.com/issues/42304.

If there is anything out there that can be considered as an enhancements or improvement on those series of tracker issues I can pick it up and push it in on this PR. If thats okay? @LenzGr @Devp00l
But some more informations are required regarding that tracker issues.

@Devp00l
Copy link

Devp00l commented Apr 22, 2020

@nizamial09 I looked over all issues and wrote comments to them. I think you should create a new PR for these issues separately but only after a conclusion was made so that you don't have to do things that are not needed.

@nizamial09-zz
Copy link
Author

@nizamial09 I looked over all issues and wrote comments to them. I think you should create a new PR for these issues separately but only after a conclusion was made so that you don't have to do things that are not needed.

Thank you so much. I'll check with the tracker issues and will update my take on it.

@nizamial09-zz
Copy link
Author

Closing this PR as this would be a regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants