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

[Fix] Support more generic tenant code when create tenant #9634

Merged
merged 8 commits into from
Apr 23, 2022
Merged

[Fix] Support more generic tenant code when create tenant #9634

merged 8 commits into from
Apr 23, 2022

Conversation

SnowMoon-Dev
Copy link
Contributor

@SnowMoon-Dev SnowMoon-Dev commented Apr 20, 2022 •
edited
Loading

… error

Purpose of the pull request

[Fix-9633][TenantManage/CreateTenant] Create a tenant is prompting an error

Brief change log

close #9633

Verify this pull request

Manually verified the change by testing locally

@SbloodyS SbloodyS added the first time contributor First-time contributor label Apr 21, 2022
@mergeable mergeable bot removed the first time contributor First-time contributor label Apr 21, 2022
@SbloodyS SbloodyS added the first time contributor First-time contributor label Apr 21, 2022
@zhongjiajie
Copy link
Member

could you add some test on it?

@SnowMoon-Dev
Copy link
Contributor Author

SnowMoon-Dev commented Apr 21, 2022 •
edited
Loading

there are already test cases, so I didn't add
@zhongjiajie

@zhongjiajie
Copy link
Member

zhongjiajie commented Apr 22, 2022 •
edited
Loading

there are already test cases, so I didn't add @zhongjiajie

I do not think so, if you change the rule, you should change the case too. In this case, you should add at least one test case with username contain char .

@zhongjiajie zhongjiajie added the miss:tests missing unit tests in PR label Apr 22, 2022
@SnowMoon-Dev
Copy link
Contributor Author

SnowMoon-Dev commented Apr 22, 2022 •
edited
Loading

i agree with you，i will add testcase

@SnowMoon-Dev
Copy link
Contributor Author

@zhongjiajie I have added test cases

@codecov-commenter
Copy link

Codecov Report

Merging #9634 (9a570c8) into dev (072ba73) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                dev    #9634      +/-   ##
============================================
- Coverage     40.01%   40.00%   -0.01%     
+ Complexity     4475     4473       -2     
============================================
  Files           835      835              
  Lines         33550    33550              
  Branches       3710     3710              
============================================
- Hits          13426    13423       -3     
- Misses        18876    18881       +5     
+ Partials       1248     1246       -2
Impacted Files Coverage Δ
.../apache/dolphinscheduler/api/utils/RegexUtils.java 100.00% <ø> (ø)
...org/apache/dolphinscheduler/remote/utils/Host.java 37.77% <0.00%> (-2.23%) ⬇️
...e/dolphinscheduler/remote/NettyRemotingClient.java 52.11% <0.00%> (-1.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 072ba73...9a570c8. Read the comment docs.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

zhongjiajie
zhongjiajie reviewed Apr 23, 2022
View reviewed changes
dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/utils/RegexUtilsTest.java
@@ -28,10 +28,10 @@ public class RegexUtilsTest {
@Test
public void testIsValidLinuxUserName() {
String name1 = "10000";
Assert.assertFalse(RegexUtils.isValidLinuxUserName(name1));
Assert.assertTrue(RegexUtils.isValidLinuxUserName(name1));
Copy link
Member

Choose a reason for hiding this comment

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

I find out that you change de previous logic, and it support the user name start with number now?

@zhongjiajie zhongjiajie removed the miss:tests missing unit tests in PR label Apr 23, 2022
@zhongjiajie zhongjiajie merged commit 29a0ea3 into apache:dev Apr 23, 2022
@zhongjiajie zhongjiajie changed the title [Fix-9633][TenantManage/CreateTenant] Create a tenant is prompting an… [Fix] Support more generic tenant code when create tenant Apr 23, 2022
fengjian1129 pushed a commit to fengjian1129/dolphinscheduler that referenced this pull request Apr 23, 2022
@SnowMoon-Dev
[Fix] Support more generic tenant code when create tenant (apache#9634)
7e07a0a
Tianqi-Dotes pushed a commit to Tianqi-Dotes/dolphinscheduler that referenced this pull request Jun 16, 2022
@SnowMoon-Dev @caishunfeng
[Fix] Support more generic tenant code when create tenant (apache#9634)
b4f9742
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhongjiajie zhongjiajie zhongjiajie approved these changes

Assignees

@SnowMoon-Dev SnowMoon-Dev

Labels
backend first time contributor First-time contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] [TenantManage/CreateTenant] Create a tenant is prompting an error message
4 participants
@SnowMoon-Dev @zhongjiajie @codecov-commenter @SbloodyS