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

Allow to edit schedules in the UI #1006

Closed
adejanovski opened this issue Jan 25, 2021 · 7 comments · Fixed by #1119
Closed

Allow to edit schedules in the UI #1006

adejanovski opened this issue Jan 25, 2021 · 7 comments · Fixed by #1119

Comments

@adejanovski
Copy link
Contributor

adejanovski commented Jan 25, 2021

Schedules can't be updated, which means they have to be deleted and recreated to change a single setting.
We need to add that feature to improve the UX.

┆Issue is synchronized with this Jira Task by Unito
┆Issue Number: K8SSAND-247

@jdonenine
Copy link
Contributor

@adejanovski I wanted to tinker a bit in the Reaper UI and figured this issue might be a decent place to start, but I had a couple of questions...

  1. Does the API to edit a schedule already exist?
  2. What fields should be editable of an already existing schedule? (the answer to question 1 might answer this if the API already exists and only exposes updates to certain fields)

Thanks!

@adejanovski
Copy link
Contributor Author

  1. Does the API to edit a schedule already exist?

No it doesn't yet.

  1. What fields should be editable of an already existing schedule? (the answer to question 1 might answer this if the API already exists and only exposes updates to certain fields)

Very good question! I think the safe (and easy path) is to allow changing everything that's specific to the schedule and avoid touching fields from the repair unit. That gives us the following fields:

  • days_between (which will force recomputing next_activation)
  • intensity
  • owner
  • repair_parallelism
  • segment_count_per_node

The new API operation should only expose these fields and in the UI we could (possibly) simplify things by using a popup exposing these fields only. I foresee some tricky stuff if we try to use the current form (which has some shared bits with the repair run form).

@jdonenine
Copy link
Contributor

I actually looked at the existing form and quickly decided I wasn't going to try to refactor that to make it support both use-cases. My thought is generally that it should, but perhaps build out the new form and then make the existing form be able to leverage it and wrap it up. The existing form does a lot of stuff it seems - a lot of stuff that I have no idea about :-)

@adejanovski
Copy link
Contributor Author

I agree, there's a lot of logic in there that will make it hard to repurpose for schedule edition.

@jdonenine
Copy link
Contributor

So let me know what you think about this...

My thinking here is to create a new form, that functions like the existing one, but that can be made a bit more flexible. At the moment it's fairly rigid in what fields are available, but that could be enhanced later for reuse.

Adds an "Edit" launch for each schedule:

image

Launches a dialog with the form, only the fields that you mentioned before will be editable:

image

image

It gives some room to enable editing of other fields later if that's something that we'd need, and should be pretty re-usable for the add workflow.

I've re-worked the visual layout slightly to be a bit more consistent across different devices too.

image

@adejanovski
Copy link
Contributor Author

Awesome stuff @jdonenine!
One tiny suggestion: in the modal header, would it be possible to put the schedule id instead of <cluster>/<keyspace>?
While the id is not easy to read (it's a timeuuid), it'll be more accurate as a primary key since you could have several schedules for the same keyspace.
Other than that, it looks great to me 👍
Thanks!

@jdonenine
Copy link
Contributor

We can for sure include the ID, I went with the approach that I did though because the ID isn't referenced in the table at all, so I don't think that having the ID there really adds much association to things. The other challenge there is the UUID is long and won't play well on smaller device resolutions. Although admittedly even a longer "name" will look poor there as well.

I'll play around with it and see how it looks, the other thing I'd suggest is that we could add the UUID inside the modal content and not in the header itself?

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

Successfully merging a pull request may close this issue.

2 participants