Skip to content

mgr/dashboard: Edit a service feature #43903

Merged
epuertat merged 1 commit intoceph:masterfrom
rhcs-dashboard:edit-service-feature
Nov 15, 2021
Merged

mgr/dashboard: Edit a service feature #43903
epuertat merged 1 commit intoceph:masterfrom
rhcs-dashboard:edit-service-feature

Conversation

@nizamial09
Copy link
Member

@nizamial09 nizamial09 commented Nov 12, 2021

Fixes: https://tracker.ceph.com/issues/53077
Signed-off-by: Nizamudeen A nia@redhat.com

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
  • Teuthology
    • Completed teuthology run
    • No teuthology test necessary (e.g., documentation)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • 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 cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

Fixes: https://tracker.ceph.com/issues/53077
Signed-off-by: Nizamudeen A <nia@redhat.com>
@nizamial09 nizamial09 requested a review from a team as a code owner November 12, 2021 08:22
@nizamial09 nizamial09 requested review from avanthakkar and pereman2 and removed request for a team November 12, 2021 08:22
@nizamial09
Copy link
Member Author

jenkins test dashboard

@nizamial09 nizamial09 changed the title mgr/dashboard: Edit a service feature mgr/dashboard: Edit a service feature Nov 12, 2021
@nizamial09
Copy link
Member Author

jenkins test dashboard

@nizamial09
Copy link
Member Author

jenkins test dashboard cephadm

1 similar comment
@pereman2
Copy link
Contributor

jenkins test dashboard cephadm

@pereman2
Copy link
Contributor

jenkins test dashboard

Copy link
Contributor

@pereman2 pereman2 left a comment

Choose a reason for hiding this comment

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

Working fine locally.

There is one nit that I don't like. When you select a service you cant edit that service, but if you want to create another service after selecting a service, you have to create it from the dropdown or refresh the window.

Usually when you click outside the table in other apps, the selection gets unselected.

@nizamial09
Copy link
Member Author

When you select a service you cant edit that service

Can't edit?

Usually when you click outside the table in other apps, the selection gets unselected.

Yup, i got that. Its happening in most of the table. @pereman2 would you create a tracker describing this issue. Might be a small issue that can be dealt with

Copy link
Contributor

@avanthakkar avanthakkar left a comment

Choose a reason for hiding this comment

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

Tested locally, editing works fine

Screencast.from.11-12-2021.06.15.40.PM.mp4

Btw, for ingress service I think some logic for editing count should be implemented bcz it creates 2 daemons for each rgw daemon (ha-proxy & keep-alived) so maybe we first of all shouldn't allow to decrease the count and for increment it should always be an even number. What you think @sebastian-philipp ?

Logic: count >= 2 * rgw_daemons_running && count%2===0

@pereman2
Copy link
Contributor

pereman2 commented Nov 12, 2021

When you select a service you cant edit that service

Can't edit?

Brain fart, can

Usually when you click outside the table in other apps, the selection gets unselected.

Yup, i got that. Its happening in most of the table. @pereman2 would you create a tracker describing this issue. Might be a small issue that can be dealt with

Sure.
update: https://tracker.ceph.com/issues/53244

Comment on lines +223 to +232
this.action = this.actionLabels.CREATE;
if (this.router.url.includes('services/(modal:create')) {
this.pageURL = 'services';
} else if (this.router.url.includes('services/(modal:edit')) {
this.editing = true;
this.pageURL = 'services';
this.route.params.subscribe((params: { type: string; name: string }) => {
this.serviceName = params.name;
this.serviceType = params.type;
});
Copy link
Member

Choose a reason for hiding this comment

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

I was recently thinking if there would be a way to pass router params as component @Inputs... And found this library and a blog post explaining how it works.

Comment on lines +262 to +267
if (this.editing) {
this.action = this.actionLabels.EDIT;
this.disableForEditing(this.serviceType);
this.cephServiceService.list(this.serviceName).subscribe((response: CephServiceSpec[]) => {
const formKeys = ['service_type', 'service_id', 'unmanaged'];
formKeys.forEach((keys) => {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for my ignorance with this part of the code, but I'm a bit astonished about how complicate is to populate the form... We have lots of back-end data harcoded here (service_type, service_id, etc). Couldn't we get this info from the back-end? Can't we use a UI API endpoint to provide all this information already processed from the back-end, so that the front-end only has to "render" it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think its possible. I was trying it out locally but so far not successful, but I'd like to try it out. I've created a tracker and I'll work on it in separate PR because the changes will be out of scope of this PR and will be messier. I hope its okay @epuertat

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.

LGTM! Thanks @nizamial09

@epuertat epuertat merged commit 689c213 into ceph:master Nov 15, 2021
@epuertat epuertat deleted the edit-service-feature branch November 15, 2021 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants