-
Notifications
You must be signed in to change notification settings - Fork 6k
mgr/dashboard: Administration > Configuration > rgw_dns_name and rgw_dns_s3website_name not updatable via dashboard #60798
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: Administration > Configuration > rgw_dns_name and rgw_dns_s3website_name not updatable via dashboard #60798
Conversation
915b4d1
to
13eb9f0
Compare
13eb9f0
to
d8f4b4e
Compare
jenkins retest this please |
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.
@nmunet left a few comments here. overall good but there are more room for improvements. thanks
...ybind/mgr/dashboard/frontend/src/app/ceph/cluster/configuration/configuration.component.html
Outdated
Show resolved
Hide resolved
d8f4b4e
to
986ecaf
Compare
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, thank you @nmunet
...ybind/mgr/dashboard/frontend/src/app/ceph/cluster/configuration/configuration.component.html
Outdated
Show resolved
Hide resolved
...ybind/mgr/dashboard/frontend/src/app/ceph/cluster/configuration/configuration.component.scss
Outdated
Show resolved
Hide resolved
src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/configuration/configuration.component.ts
Outdated
Show resolved
Hide resolved
src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/configuration/configuration.component.ts
Outdated
Show resolved
Hide resolved
986ecaf
to
923758c
Compare
jenkins test make check |
jenkins test api |
jenkins test submodules |
jenkins test dashboard |
jenkins test api |
jenkins test dashboard |
jenkins test api |
2 similar comments
jenkins test api |
jenkins test api |
3c73d49
to
a25916c
Compare
a25916c
to
d80872f
Compare
d80872f
to
f3ca718
Compare
jenkins test api |
jenkins test dashboard |
jenkins test windows |
f3ca718
to
ebe7ebc
Compare
jenkins test api |
1 similar comment
jenkins test api |
ebe7ebc
to
a0d7c87
Compare
jenkins test api |
jenkins test windows |
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.
Please fix the signed-off commit, thanks.
This is a good improvement, thanks 👍🏿
src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/configuration/configuration.component.ts
Outdated
Show resolved
Hide resolved
...ontend/src/app/ceph/cluster/configuration/configuration-form/configuration-form.component.ts
Outdated
Show resolved
Hide resolved
...ontend/src/app/ceph/cluster/configuration/configuration-form/configuration-form.component.ts
Show resolved
Hide resolved
src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/configuration/configuration.component.ts
Outdated
Show resolved
Hide resolved
3e13e0e
to
b559501
Compare
b559501
to
4bb9c7f
Compare
jenkins test make check |
src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/configuration/configuration.component.ts
Outdated
Show resolved
Hide resolved
…tions are not updatable at runtime Fixes: https://tracker.ceph.com/issues/68976 Fixes Includes: 1) by-passing 'can_update_at_runtime' flag for 'rgw' related configurations as the same can be updated at runtime via CLI. Also implemented a warning popup for user to make force edit to rgw related configurations. 2) when navigated to Administration >> Configuration, modified configuration will be seen as we see in cli "ceph config dump", instead of configuration with filter level:basic Signed-off-by: Naman Munet <[email protected]>
4bb9c7f
to
3181acc
Compare
jenkins test api |
jenkins test make check |
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.
Non blocking comments but good for maintenance.
import { CriticalConfirmationModalComponent } from '~/app/shared/components/critical-confirmation-modal/critical-confirmation-modal.component'; | ||
import { ModalCdsService } from '~/app/shared/services/modal-cds.service'; | ||
|
||
const RGW = 'rgw'; |
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 dont need to define same constant in both files.
We can move it to a generic file and call from that to both places.
This is for normal housekeeping (not blocking PR)
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.
will work on these changes in my next PR related to configuration, as we have more work to do here!
Thanks
@@ -118,7 +123,7 @@ export class ConfigurationFormComponent extends CdForm implements OnInit { | |||
this.configForm.get('values').get(value.section).setValue(sectionValue); | |||
}); | |||
} | |||
|
|||
this.forceUpdate = !this.response.can_update_at_runtime && response.name.includes(RGW); |
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.
Perhaps add a generic helper function and move this alongwith the constant in a generic file calling at both places.
this.forceUpdate = !this.response.can_update_at_runtime && response.name.includes(RGW); | |
this.forceUpdate = !this.response.can_update_at_runtime && hasRGW(response.name); |
@@ -143,7 +145,9 @@ export class ConfigurationComponent extends ListWithDetails implements OnInit { | |||
if (selection.selected.length !== 1) { | |||
return false; | |||
} | |||
|
|||
return selection.selected[0].can_update_at_runtime; | |||
if ((this.selection.selected[0].name as string).includes(RGW)) { |
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.
if ((this.selection.selected[0].name as string).includes(RGW)) { | |
if (hasRGW(this.selection.selected[0].name)) { |
Fixes: https://tracker.ceph.com/issues/68976
Signed-off-by: Naman Munet [email protected]
Previously: users are not able to edit the configuration which have the 'can_update_at_runtime' flag as false.
Now:
Fixes Includes:
Screencast.from.2024-11-22.15-33-23.mp4
Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an
x
between the brackets:[x]
. Spaces and capitalization matter when checking off items this way.Checklist
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
jenkins test windows
jenkins test rook e2e