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.

Sign up for GitHub

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

Jump to bottom

mgr/dashboard: validate mds service_id and helper text for service_id #47179

Merged
merged 1 commit into from Aug 10, 2022
Merged

mgr/dashboard: validate mds service_id and helper text for service_id #47179

merged 1 commit into from Aug 10, 2022

Conversation

melissa-kun-li
Copy link
Contributor

@melissa-kun-li melissa-kun-li commented Jul 19, 2022 •
edited

Validate mds service_id to avoid mds daemon from being created and going into error state due to invalid id.
Also add helper text to describe the role of service_id.

Fixes: https://tracker.ceph.com/issues/56077
Signed-off-by: Melissa Li melissali@redhat.com

Contribution Guidelines

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)
    • Code cleanup (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)
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

@melissa-kun-li melissa-kun-li requested a review from a team as a code owner July 19, 2022 21:21
@melissa-kun-li melissa-kun-li requested review from pereman2 and Pegonzal and removed request for a team July 19, 2022 21:21
@github-actions github-actions bot added this to In progress in Dashboard Jul 19, 2022
Dashboard automation moved this from In progress to Review in progress Jul 20, 2022
Pegonzal
Pegonzal requested changes Jul 20, 2022
View reviewed changes
Copy link
Contributor

@Pegonzal Pegonzal left a comment

Choose a reason for hiding this comment

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

LGTM @melissa-kun-li , just left some comments on a couple things before approving

...gr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.html Outdated
@@ -80,6 +82,9 @@
<span class="invalid-feedback"
*ngIf="serviceForm.showError('service_id', frm, 'rgwPattern')"
i18n>The value does not match the pattern <strong>&lt;service_id&gt;[.&lt;realm_name&gt;.&lt;zone_name&gt;]</strong>.</span>
<span class="invalid-feedback"
*ngIf="serviceForm.showError('service_id', frm, 'mdsPattern')"
i18n>MDS service_id must match the pattern <strong>^[a-zA-Z_.-][a-zA-Z0-9_.-]*$</strong></span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit:

I would prefer to have a more user-friendly string to show feedback instead of a regex pattern.
Maybe something like:

Suggested change
i18n>MDS service_id must match the pattern <strong>^[a-zA-Z_.-][a-zA-Z0-9_.-]*$</strong></span>
i18n>MDS service_id must start with a letter and and can contain alphanumeric characters, '.', '-' or '_'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point on more user-friendly string. After change, it looks like this now:
Screenshot from 2022-07-21 14-45-45

do we like this, or does it look like it could be misinterpreted as accepting apostrophes and/or commas?

Copy link
Contributor

Choose a reason for hiding this comment

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

Even though I agree it can be confusing I think that would work, we are already using it in other places.

...gr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.html Outdated Show resolved Hide resolved
...dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.spec.ts Outdated Show resolved Hide resolved
@melissa-kun-li
Copy link
Contributor Author

Thanks for the review @Pegonzal ! Updated now

@melissa-kun-li
Copy link
Contributor Author

jenkins test dashboard

@Pegonzal
Copy link
Contributor

jenkins test dashboard cephadm

pereman2
pereman2 approved these changes Jul 25, 2022
View reviewed changes
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, I left a minor comment.

...gr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.html Outdated Show resolved Hide resolved
Dashboard automation moved this from Review in progress to Reviewer approved Jul 25, 2022
@melissa-kun-li
mgr/dashboard: validate mds service_id and helper text for service_id
1d6cb28 
Validate mds service_id to avoid mds daemon from being created and going into error state due to invalid id.
Also add helper text to describe the role of service_id.

Fixes: https://tracker.ceph.com/issues/56077
Signed-off-by: Melissa Li <melissali@redhat.com>
@aaSharma14
Copy link
Contributor

jenkins test dashboard cephadm

@aaSharma14
Copy link
Contributor

jenkins test windows

@nizamial09
Copy link
Member

jenkins test windows

@nizamial09 nizamial09 moved this from Reviewer approved to Ready-to-merge in Dashboard Aug 4, 2022
@nizamial09
Copy link
Member

jenkins test dashboard cephadm

@pereman2
Copy link
Contributor

Cehpadm tests are unrelated

Test Result (1 failure / +1)
should check if mon daemon is running on all hosts.when cluster creation is completed Hosts page should check if mon daemon is running on all hosts

Identified problems
Dashboard End-to-End Test Failure (Cypress)

1 of 10 failed (10%)
Indication 1
No such file or directory

No such file or directory
Indication 2
Python Assertion Error

Python Assertion Error (probably from a Unit Test)
Indication 3

@pereman2 pereman2 merged commit 2e03b79 into ceph:main Aug 10, 2022
Dashboard automation moved this from Ready-to-merge to Done Aug 10, 2022
@pereman2 pereman2 deleted the service-id-info branch August 10, 2022 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pereman2 pereman2 pereman2 approved these changes

@aaSharma14 aaSharma14 aaSharma14 approved these changes

@Pegonzal Pegonzal Pegonzal approved these changes

Assignees
No one assigned
Labels
dashboard pybind
Projects
Ceph-Dashboard
Archived in project
Dashboard
  
Done
Milestone
No milestone
5 participants
@melissa-kun-li @Pegonzal @aaSharma14 @nizamial09 @pereman2