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

Add spotless support #3171

Merged
merged 14 commits into from
Jul 11, 2023
Merged

Conversation

Leo6Leo
Copy link
Contributor

@Leo6Leo Leo6Leo commented Jun 26, 2023

Fixes #3166

Proposed Changes

  • Added the support for Spotless
  • Automate the formatting by integrating this with update-codegen.sh
  • Add the format check to the CI testing set

Release Note

Added Spotless as the Java formatter support

@knative-prow
Copy link

knative-prow bot commented Jun 26, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@knative-prow knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/data-plane labels Jun 26, 2023
@knative-prow knative-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 26, 2023
@Leo6Leo Leo6Leo marked this pull request as ready for review June 27, 2023 14:36
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 27, 2023
@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Jun 27, 2023

/cc @pierDipi I will push the code changes for those reformatted .java file if you don't see any problem with the all the changes I have made so far!

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2023
@knative-prow knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 27, 2023
@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Jun 27, 2023

/retest

@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2023
@knative-prow knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 27, 2023
@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Merging #3171 (7a641d9) into main (e8ba86f) will increase coverage by 0.03%.
The diff coverage is 80.71%.

@@             Coverage Diff              @@
##               main    #3171      +/-   ##
============================================
+ Coverage     63.42%   63.46%   +0.03%     
+ Complexity      755      753       -2     
============================================
  Files           167      167              
  Lines         11805    11815      +10     
  Branches        241      246       +5     
============================================
+ Hits           7487     7498      +11     
+ Misses         3754     3750       -4     
- Partials        564      567       +3     
Flag Coverage Δ
java-unittests 79.88% <80.71%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
control-plane/pkg/contract/contract.pb.go 7.44% <ø> (ø)
...nting/kafka/broker/dispatcher/ResponseHandler.java 0.00% <0.00%> (ø)
...ka/broker/dispatcher/impl/NoopResponseHandler.java 0.00% <0.00%> (ø)
...ng/kafka/broker/dispatcher/main/DispatcherEnv.java 0.00% <0.00%> (ø)
...ve/eventing/kafka/broker/dispatcher/main/Main.java 0.00% <0.00%> (ø)
...venting/kafka/broker/receiver/IngressProducer.java 100.00% <ø> (ø)
...eventing/kafka/broker/receiver/RequestContext.java 100.00% <ø> (ø)
...eceiver/impl/IngressProducerReconcilableStore.java 83.50% <ø> (-0.66%) ⬇️
...g/kafka/broker/receiver/impl/ReceiverVerticle.java 68.57% <ø> (+2.85%) ⬆️
...ker/receiver/impl/StrictRequestToRecordMapper.java 75.00% <ø> (ø)
... and 67 more

@knative-prow knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 27, 2023
@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Jun 27, 2023

/retest

@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Jun 27, 2023

/meow

@knative-prow
Copy link

knative-prow bot commented Jun 27, 2023

@Leo6Leo: cat image

In response to this:

/meow

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Jun 27, 2023

@pierDipi Currently, the maven version on my local machine is 3.8.6, but on GitHub the maven version is 3.8.3. I am wondering that should we bump up the maven version, or I downgrade to 3.8.3 instead?

#1301

@debasishbsws
Copy link
Member

debasishbsws commented Jun 27, 2023

The google.java.format uses 2 spaces as indentation by default for Java code, which I don't like personally. Is it possible to overwrite it?
Or we can use palantir-java-format which is a fork of google.java.format with some better features IG, example. It may also reduce code changes in this PR as well.

Let's see what others have to say about this.

@pierDipi
Copy link
Member

@pierDipi Currently, the maven version on my local machine is 3.8.6, but on GitHub the maven version is 3.8.3. I am wondering that should we bump up the maven version, or I downgrade to 3.8.3 instead?

#1301

What's the problem with that?

@pierDipi
Copy link
Member

The google.java.format uses 2 spaces as indentation by default for Java code, which I don't like personally. Is it possible to overwrite it? Or we can use palantir-java-format which is a fork of google.java.format with some better features IG, example. It may also reduce code changes in this PR as well.

Let's see what others have to say about this.

Regarding the format, I don't have strong opinions, I just like a consistent style, the shared link does make valid arguments in favor of the Palantir format, up to you all on which format you think is the best for our project

@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Jul 4, 2023

/retest

1 similar comment
@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Jul 4, 2023

/retest

@debasishbsws
Copy link
Member

/retest-required

@debasishbsws
Copy link
Member

I think the build-tests is failing because you have added a new module and that should be reflected in /data-plane/THIRD-PARTY.txt
Run ./hack/update-codegen.sh And commit the modified THIRD-PARTY.txt file

@pierDipi
Copy link
Member

pierDipi commented Jul 6, 2023

This patch might fix the problem:

diff --git a/hack/update-codegen.sh b/hack/update-codegen.sh
index af3b8106a..d77b1000b 100755
--- a/hack/update-codegen.sh
+++ b/hack/update-codegen.sh
@@ -77,14 +77,13 @@ group "Update deps post-codegen"
 if ! ${GITHUB_ACTIONS:-false}; then
   "${REPO_ROOT_DIR}"/hack/generate-proto.sh
 
+  pushd "${REPO_ROOT_DIR}/data-plane"
   # Update Java third party file
-  pushd data-plane
   ./mvnw -Dlicense.outputDirectory=. license:aggregate-add-third-party
-  popd
-
   # Run maven command to apply spotless formatting
-  pushd "${REPO_ROOT_DIR}/data-plane"
-  mvn spotless:apply
+  ./mvnw spotless:apply
+  # Clean target directories
+  ./mvnw clean
   popd
 fi
 
...skipping...
+  ./mvnw clean
   popd
 fi
 
diff --git a/hack/verify-codegen.sh b/hack/verify-codegen.sh
index 1286dc245..2766c5a71 100755
--- a/hack/verify-codegen.sh
+++ b/hack/verify-codegen.sh
@@ -38,7 +38,13 @@ mkdir -p "${TMP_DIFFROOT}"
 DIRS=(
   "/go.mod"
   "/go.sum"
-  "/data-plane"
+  "/data-plane/contract"
+  "/data-plane/core"
+  "/data-plane/dispatcher"
+  "/data-plane/dispatcher-vertx"
+  "/data-plane/receiver"
+  "/data-plane/receiver-vertx"
+  "/data-plane/tests"
   "/control-plane/pkg/core/config"
   "/control-plane/pkg/apis"
   "/control-plane/pkg/client"

@pierDipi
Copy link
Member

pierDipi commented Jul 6, 2023

Because the error says that the target directory and binary files are different so we need to avoid comparing those

@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Jul 6, 2023

/cc @pierDipi Ready for review

@knative-prow
Copy link

knative-prow bot commented Jul 6, 2023

@Leo6Leo: GitHub didn't allow me to request PR reviews from the following users: Ready, for, review.

Note that only knative-sandbox members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @pierDipi Ready for review

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Jul 6, 2023

/retest

hack/update-codegen.sh Outdated Show resolved Hide resolved
hack/update-codegen.sh Outdated Show resolved Hide resolved
Leo6Leo and others added 3 commits July 10, 2023 09:06
Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2023
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2023
@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Jul 10, 2023

/retest

1 similar comment
@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Jul 10, 2023

/retest

@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Jul 10, 2023

/cc @pierDipi

Ready for review again

@knative-prow knative-prow bot requested a review from pierDipi July 10, 2023 17:01
@pierDipi
Copy link
Member

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2023
@knative-prow
Copy link

knative-prow bot commented Jul 11, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Leo6Leo, pierDipi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 11, 2023
@knative-prow knative-prow bot merged commit 8a22c57 into knative-extensions:main Jul 11, 2023
35 of 37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane area/data-plane lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Java Code Formatter
5 participants