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.

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

feat(CI): Added GitHub Actions to check for code style violations #13489

Merged
merged 41 commits into from
Jan 18, 2024

Conversation

SaptarshiSarkar12
Copy link
Contributor

@SaptarshiSarkar12 SaptarshiSarkar12 commented Dec 12, 2023

What is the purpose of the change

Closes #13413

Brief changelog

  • Added GitHub Actions to check for code style violations.

Checklist

  • Make sure there is a GitHub_issue field for the change (usually before you start working on it). Trivial changes like typos do not require a GitHub issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Check if is necessary to patch to Dubbo 3 if you are work on Dubbo 2.7
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
  • Add some description to dubbo-website project if you are requesting to add a feature.
  • GitHub Actions works fine on your own branch.
  • If this contribution is large, please follow the Software Donation Guide.

@SaptarshiSarkar12
Copy link
Contributor Author

/format

@codecov-commenter
Copy link

codecov-commenter commented Dec 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e583187) 70.38% compared to head (7ebf0a4) 70.35%.

❗ Current head 7ebf0a4 differs from pull request most recent head ecee9f5. Consider uploading reports for the commit ecee9f5 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##              3.2   #13489      +/-   ##
==========================================
- Coverage   70.38%   70.35%   -0.04%     
==========================================
  Files        1606     1606              
  Lines       69999    69999              
  Branches    10098    10098              
==========================================
- Hits        49272    49248      -24     
- Misses      16103    16115      +12     
- Partials     4624     4636      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SaptarshiSarkar12
Copy link
Contributor Author

@AlbumenJ I have added a slight tab to break the style of one of the java file to check if the CI actually works.

@SaptarshiSarkar12
Copy link
Contributor Author

@AlbumenJ I think the workflows don't have read and write permission. You can go to Settings > Actions > general and turn on this option 👇
image

@SaptarshiSarkar12
Copy link
Contributor Author

@AlbumenJ I think the workflows don't have read and write permission. You can go to Settings > Actions > general and turn on this option 👇 image

@AlbumenJ The workflow will fail unless you set these permissions.

@AlbumenJ
Copy link
Member

@AlbumenJ I think the workflows don't have read and write permission. You can go to Settings > Actions > general and turn on this option 👇 image

@AlbumenJ The workflow will fail unless you set these permissions.

Seems it should be changed to scheduled task to scan PRs due to permission limitation. The ASF will not enable write access in PRs due to workflows file can be changed by anyone. So it might be a good idea to switch to other kinds of task.

@SaptarshiSarkar12
Copy link
Contributor Author

@AlbumenJ Okay, I see. So, do you mean that this workflow should run on schedule and will just check if codestyle is followed?

@AlbumenJ
Copy link
Member

@AlbumenJ Okay, I see. So, do you mean that this workflow should run on schedule and will just check if codestyle is followed?

Yes :)

@SaptarshiSarkar12
Copy link
Contributor Author

@AlbumenJ Okay. Then, what should be the time of the workflow run in UTC timezone (this timezone is followed by GH Actions)? Should I keep it at midnight UTC everyday? You can check and give me a custom cron time from https://crontab.guru/ .

@AlbumenJ
Copy link
Member

@AlbumenJ Okay. Then, what should be the time of the workflow run in UTC timezone (this timezone is followed by GH Actions)? Should I keep it at midnight UTC everyday? You can check and give me a custom cron time from https://crontab.guru/ .

Every hour or even 20 mins are both ok

@SaptarshiSarkar12
Copy link
Contributor Author

Okay @AlbumenJ

@SaptarshiSarkar12
Copy link
Contributor Author

@AlbumenJ I have added the cron job in the workflow to run at every 20 mins.

@SaptarshiSarkar12
Copy link
Contributor Author

@AlbumenJ The cron job worked 🎉 - you can check here - https://github.com/SaptarshiSarkar12/dubbo/actions/runs/7330812949

@CrazyHZM
Copy link
Member

@SaptarshiSarkar12
When PR is submitted, the existing ci process will automatically execute mvn spotless check due to the execution of mvn install. I can't think of the function of this schedule. The code format is guaranteed because this check is done for every commit.

I have an individual suggestion: add a ci code checkstyle to each submission instead of completing it through schedule.

@SaptarshiSarkar12
Copy link
Contributor Author

@CrazyHZM Okay. But, I couldn't understand your suggestion. Can you please elaborate?

@SaptarshiSarkar12
Copy link
Contributor Author

@AlbumenJ Should I add the Check for formatting workflow to Build and Test Schedule also?

@AlbumenJ
Copy link
Member

@AlbumenJ Should I add the Check for formatting workflow to Build and Test Schedule also?

I think it can be omitted. Because the scheduled task itself will check the format.

@SaptarshiSarkar12
Copy link
Contributor Author

@AlbumenJ Should the workflow remain in the Build and test PR workflow? I couldn't understand your words. Can you please elaborate?

@AlbumenJ
Copy link
Member

@AlbumenJ Should the workflow remain in the Build and test PR workflow? I couldn't understand your words. Can you please elaborate?

Sorry about that. The workflow of Build and Test Schedule has already contained the check the code style with spotless now.
Maybe in

run: ./mvnw --batch-mode --no-snapshot-updates -e --no-transfer-progress --fail-fast clean test verify -Pjacoco -Dmaven.wagon.httpconnectionManager.ttlSeconds=120 -Dmaven.wagon.http.retryHandler.count=5 -DskipTests=false -DskipIntegrationTests=false -Dcheckstyle.skip=false -Dcheckstyle_unix.skip=false -Drat.skip=false -Dmaven.javadoc.skip=true -DembeddedZookeeperPath=${{ github.workspace }}/.tmp/zookeeper

@SaptarshiSarkar12
Copy link
Contributor Author

SaptarshiSarkar12 commented Jan 11, 2024

@AlbumenJ It's okay.

@SaptarshiSarkar12
Copy link
Contributor Author

@AlbumenJ Please review this PR then. Let me know if any further changes are required.

@SaptarshiSarkar12
Copy link
Contributor Author

@AlbumenJ I don't think the scheduled check creates summary if linting fails. Can I try adding it?

@AlbumenJ
Copy link
Member

@AlbumenJ Please review this PR then. Let me know if any further changes are required.

This PR LGTM

@AlbumenJ
Copy link
Member

@AlbumenJ I don't think the scheduled check creates summary if linting fails. Can I try adding it?

Build and Test For PR is enough now. scheduled check is for maintainers.

@SaptarshiSarkar12
Copy link
Contributor Author

Okay @AlbumenJ. So, can I remove my changes which were made solely for testing the CI (like adding extra spaces)?

@AlbumenJ
Copy link
Member

Okay @AlbumenJ. So, can I remove my changes which were made solely for testing the CI (like adding extra spaces)?

Yep

@SaptarshiSarkar12
Copy link
Contributor Author

Okay @AlbumenJ. Would do it soon.
Thank you!

@SaptarshiSarkar12
Copy link
Contributor Author

@AlbumenJ I have made the required changes.

Copy link

sonarcloud bot commented Jan 18, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link
Member

@AlbumenJ AlbumenJ left a comment

Choose a reason for hiding this comment

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

LGTM

@AlbumenJ
Copy link
Member

@CrazyHZM PTAL

@AlbumenJ AlbumenJ merged commit f20fac4 into apache:3.2 Jan 18, 2024
19 checks passed
@AlbumenJ
Copy link
Member

@SaptarshiSarkar12 Thanks for your contribution :)

@SaptarshiSarkar12
Copy link
Contributor Author

@AlbumenJ I have a question. What is the full form / meaning of PTAL?
@AlbumenJ @CrazyHZM Thank you for reviewing and merging my PR 😄! I loved working on this project ❤️.

@liaozan
Copy link
Contributor

liaozan commented Jan 18, 2024

PTAL Please take a look

@SaptarshiSarkar12
Copy link
Contributor Author

Okay. Got it. Thank you @liaozan 😄!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: Add Auto-Format Workflow for Java files
5 participants