-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[fix][admin] Tenant AdminRoles can not contains whitespace in the beginning or end. #22450
Conversation
please help review, thanks. @BewareMyPower @poorbarcode @codelipenghui @liangyepianzhou |
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.
Add fail
to avoid test passing even if the call succeeds
pulsar-broker/src/test/java/org/apache/pulsar/client/api/TenantTest.java
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/client/api/TenantTest.java
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java
Outdated
Show resolved
Hide resolved
please review, thanks. @BewareMyPower @poorbarcode @liangyepianzhou |
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 checkstyle
https://github.com/apache/pulsar/actions/runs/8624709432/job/23654223248?pr=22450
|
Signed-off-by: falser101 <zjf_0731@163.com>
Signed-off-by: falser101 <zjf_0731@163.com>
Signed-off-by: falser101 <zjf_0731@163.com>
please help review @lhotari @liangyepianzhou |
I think it would be ready to merge if tests passed since all comments should be addressed. |
/test |
how to run test,it's waiting for status to be reported? |
Since you're the first-time contributor, the workflow requires a committer to click the |
There are some failed tests, please check them. |
ok. |
resolved |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #22450 +/- ##
============================================
- Coverage 73.57% 73.21% -0.36%
- Complexity 32624 32902 +278
============================================
Files 1877 1890 +13
Lines 139502 141502 +2000
Branches 15299 15528 +229
============================================
+ Hits 102638 103606 +968
- Misses 28908 29903 +995
- Partials 7956 7993 +37
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@BewareMyPower help merge this pull requests,thanks |
Fixes #21710
Verifying this change
(Please pick either of the following options)
This change added tests
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc
doc-required
doc-not-needed
doc-complete