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

[Feature-10983] [refactor] ResourcesController #10998

Closed
wants to merge 1 commit into from
Closed

[Feature-10983] [refactor] ResourcesController #10998

wants to merge 1 commit into from

Conversation

liguotian
Copy link

@liguotian liguotian commented Jul 15, 2022 •
edited
Loading

refactor for api ResourcesController
fix #10983

@liguotian
Copy link
Author

I have reactor the code and supplement UnitTest。but when i mvn package in my compute ,there has some stylecheck error like ImportOrder in java where i haven't edit.

@SbloodyS SbloodyS added the first time contributor First-time contributor label Jul 16, 2022
@liguotian
Copy link
Author

I‘m sorry to forget to add license text for new class， so I repush the code

@liguotian
Copy link
Author

I have fix all check-style error with incremental code @SbloodyS

@SbloodyS
Copy link
Member

I had approved to run CI.

@liguotian
Copy link
Author

I had approved to run CI.

it is confusing to me that i have checked by check-style plugin in my local , it also has no problem .Do you have any suggestions?

@SbloodyS
Copy link
Member

I had approved to run CI.

it is confusing to me that i have checked by check-style plugin in my local , it also has no problem .Do you have any suggestions?

The current CI is incremental detection. Please check all files you have modified.

@liguotian
Copy link
Author

OK, it seems that check-style plugin skip the Test package

@liguotian liguotian force-pushed the Feature-10983 branch 2 times, most recently from 198882f to a88d4bb Compare July 19, 2022 04:10
@liguotian
Copy link
Author

I had approved to run CI.

it is confusing to me that i have checked by check-style plugin in my local , it also has no problem .Do you have any suggestions?

The current CI is incremental detection. Please check all files you have modified.

I try to fix check-style error ,please help to approve it

@liguotian
Copy link
Author

@caishunfeng will you please help to review code

SbloodyS
SbloodyS reviewed Jul 20, 2022
View reviewed changes
...eduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/ResourcesController.java Outdated Show resolved Hide resolved
...uler-api/src/main/java/org/apache/dolphinscheduler/api/controller/ResourcesV2Controller.java Outdated Show resolved Hide resolved
...uler-api/src/main/java/org/apache/dolphinscheduler/api/controller/ResourcesV2Controller.java Outdated Show resolved Hide resolved
...uler-api/src/main/java/org/apache/dolphinscheduler/api/controller/ResourcesV2Controller.java Outdated Show resolved Hide resolved
...uler-api/src/main/java/org/apache/dolphinscheduler/api/controller/ResourcesV2Controller.java Outdated Show resolved Hide resolved
...uler-api/src/main/java/org/apache/dolphinscheduler/api/controller/ResourcesV2Controller.java Outdated Show resolved Hide resolved
...uler-api/src/main/java/org/apache/dolphinscheduler/api/controller/ResourcesV2Controller.java Outdated Show resolved Hide resolved
...-api/src/main/java/org/apache/dolphinscheduler/api/dto/resources/CreateDirectoryRequest.java Outdated Show resolved Hide resolved
@SbloodyS SbloodyS added this to the 4.0.0-alpha milestone Jul 20, 2022
@liguotian liguotian force-pushed the Feature-10983 branch 2 times, most recently from 314d16c to 37e749c Compare July 20, 2022 17:07
@liguotian
Copy link
Author

@SbloodyS e2e MysqlDataSource module error , I haven't change it ，is it because I pull the lastest commit ?

@SbloodyS
Copy link
Member

@SbloodyS e2e MysqlDataSource module error , I haven't change it ，is it because I pull the lastest commit ?

Please merge the latest dev branch again. This problem was just fixed this afternoon.

@liguotian
Copy link
Author

Done

@SbloodyS
Copy link
Member

@SbloodyS Can you help to approval 。it seems this PR will outdate

I had approved to run CI.

@liguotian
Copy link
Author

@SbloodyS Can you help to approval 。it seems this PR will outdate

I had approved to run CI.

@SbloodyS Sorry ,I had e a mistake in code. now i had fix it .i find i can run action in my repo to check ALL UNIT Test , now
i had run action in my own forked repo and they all success, please approved again . I'm sorry for the trouble

@SbloodyS
Copy link
Member

@SbloodyS Can you help to approval 。it seems this PR will outdate

I had approved to run CI.

@SbloodyS Sorry ,I had e a mistake in code. now i had fix it .i find i can run action in my repo to check ALL UNIT Test , now i had run action in my own forked repo and they all success, please approved again . I'm sorry for the trouble

It's alright. I had rerun the CI.

@liguotian
Copy link
Author

@SbloodyS Can you help to approval 。it seems this PR will outdate

I had approved to run CI.

@SbloodyS Sorry ,I had e a mistake in code. now i had fix it .i find i can run action in my repo to check ALL UNIT Test , now i had run action in my own forked repo and they all success, please approved again . I'm sorry for the trouble

It's alright. I had rerun the CI.

Thanks!

@liguotian
Copy link
Author

@SbloodyS I can see that there is still a check failed, but the status of the RP is "review required". Is this check important? How do I do it? Can you help me

@liguotian liguotian requested review from caishunfeng and SbloodyS and removed request for caishunfeng August 22, 2022 03:52
@liguotian
Copy link
Author

@SbloodyS Can you help to review code for PR ？

@liguotian
Copy link
Author

@SbloodyS Excuse me ，but can you help approving

@SbloodyS
Copy link
Member

@SbloodyS Excuse me ，but can you help approving

Sorry for late reply. I had approved to run CI.

@sonarcloud
Copy link

sonarcloud bot commented Sep 13, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug D 2 Bugs
Vulnerability B 1 Vulnerability
Security Hotspot A 0 Security Hotspots
Code Smell A 136 Code Smells

50.1% 50.1% Coverage
22.9% 22.9% Duplication

@liguotian
Copy link
Author

@SbloodyS Excuse me ，but can you help approving

Sorry for late reply. I had approved to run CI.

It’s no matter。can you help CR。

@liguotian
Copy link
Author

@SbloodyS can you please review code ?

@SbloodyS
Copy link
Member

@SbloodyS can you please review code ?

Sorry for late reply. I can't spare time these days. Do you have time to take a look at this PR? Thanks. @EricGao888 @caishunfeng @zhongjiajie @ruanwenjun @zhuangchong

@EricGao888 EricGao888 self-requested a review September 26, 2022 06:33
@EricGao888
Copy link
Member

@SbloodyS can you please review code ?

Sorry for late reply. I can't spare time these days. Do you have time to take a look at this PR? Thanks. @EricGao888 @caishunfeng @zhongjiajie @ruanwenjun @zhuangchong

Sure, I will help with the review : )

@liguotian
Copy link
Author

@EricGao888 Excuse me， Can this PR still be review 。

@EricGao888
Copy link
Member

@EricGao888 Excuse me， Can this PR still be review 。

Of course.

zhongjiajie
zhongjiajie reviewed Oct 20, 2022
View reviewed changes
...-api/src/main/java/org/apache/dolphinscheduler/api/dto/resources/UpdateResourceResponse.java
Comment on lines +25 to +45
public class UpdateResourceResponse extends Result<Object> {

private Object data;

public UpdateResourceResponse(Result<Object> result) {
super();
this.setCode(result.getCode());
this.setMsg(result.getMsg());
this.setData(result.getData());
}

@Override
public Object getData() {
return data;
}

@Override
public void setData(Object data) {
this.data = data;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

about all the response, is an other Result object, maybe we can directly use result object, we already have some example, you can see ScheduleV2Controller as example

Copy link
Author

Choose a reason for hiding this comment

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

i see ScheduleV2Controller refer service method with V2 。So do I have to do the same. or i just to return result object like Result<Object> | Map<String, Object>`

...-api/src/test/java/org/apache/dolphinscheduler/api/controller/ResourcesV2ControllerTest.java Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2023

This pull request has been automatically marked as stale because it has not had recent activity for 120 days. It will be closed in 7 days if no further activity occurs.

@github-actions github-actions bot added the Stale label Nov 7, 2023
@github-actions GitHub Actions
Copy link

This pull request has been closed because it has not had recent activity. You could reopen it if you try to continue your work, and anyone who are interested in it are encouraged to continue work on this pull request.

@github-actions github-actions bot closed this Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhongjiajie zhongjiajie zhongjiajie left review comments

@SbloodyS SbloodyS Awaiting requested review from SbloodyS SbloodyS is a code owner

@EricGao888 EricGao888 Awaiting requested review from EricGao888

@caishunfeng caishunfeng Awaiting requested review from caishunfeng caishunfeng is a code owner

Assignees

@liguotian liguotian

Labels
backend first time contributor First-time contributor refactor Stale
Projects
None yet
Milestone
4.0.0-alpha
Development

Successfully merging this pull request may close these issues.

[Feature][Api] Refactor org.apache.dolphinscheduler.api.controller.ResourcesController
7 participants
@liguotian @SbloodyS @ruanwenjun @zhongjiajie @caishunfeng @codecov-commenter @EricGao888