chore(CI): adds googlejavaformat action to CI.

[jcchavezs]

This PR adds a google java format check on CI.

[codecov]

Codecov Report

Merging #24 (ab05c94) into main (cd6c5a9) will increase coverage by 0.04%.
The diff coverage is 69.64%.

@@             Coverage Diff              @@
##               main      #24      +/-   ##
============================================
+ Coverage     69.44%   69.49%   +0.04%     
  Complexity      160      160              
============================================
  Files            11       11              
  Lines           707      708       +1     
  Branches         73       73              
============================================
+ Hits            491      492       +1     
  Misses          174      174              
  Partials         42       42              

Flag	Coverage Δ	Complexity Δ
integration	57.06% <56.97%> (-0.09%)	0.00 <156.00> (ø)
unit	31.07% <31.07%> (+0.09%)	0.00 <156.00> (ø)

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

Impacted Files	Coverage Δ	Complexity Δ
...rtrace/core/documentstore/DocumentStoreConfig.java	0.00% <ø> (ø)	0.00 <0.00> (ø)
...rg/hypertrace/core/documentstore/JSONDocument.java	56.52% <56.52%> (ø)	6.00 <6.00> (ø)
...core/documentstore/postgres/PostgresDatastore.java	57.62% <56.89%> (ø)	10.00 <9.00> (ø)
.../hypertrace/core/documentstore/SingleValueKey.java	57.14% <57.14%> (ø)	6.00 <6.00> (ø)
...ava/org/hypertrace/core/documentstore/OrderBy.java	60.00% <60.00%> (ø)	3.00 <3.00> (ø)
.../java/org/hypertrace/core/documentstore/Query.java	64.00% <60.86%> (ø)	10.00 <9.00> (ø)
...java/org/hypertrace/core/documentstore/Filter.java	69.56% <69.56%> (+0.44%)	18.00 <18.00> (ø)
...race/core/documentstore/mongo/MongoCollection.java	69.74% <70.46%> (ø)	41.00 <41.00> (ø)
...ore/documentstore/postgres/PostgresCollection.java	72.45% <72.41%> (ø)	53.00 <52.00> (ø)
...pertrace/core/documentstore/DatastoreProvider.java	78.57% <83.33%> (ø)	3.00 <3.00> (ø)
... and 1 more

---

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 cd6c5a9...ab05c94. Read the comment docs.

[jcchavezs]

@aaron-steinfeld @buchi-busireddy could you please loop in and tell what are the rules we expect to meet? also the version for the formatting rules?

[aaron-steinfeld]

@aaron-steinfeld @buchi-busireddy could you please loop in and tell what are the rules we expect to meet? also the version for the formatting rules?

We generally use https://plugins.jetbrains.com/plugin/8527-google-java-format - so it looks like that currently means 1.9 (I think their versioning is not of the rules themselves which don't appear to be versioned https://google.github.io/styleguide/javaguide.html , but the tool's implementation?)

[aaron-steinfeld]

Do we need ignore paths? This check takes ~15s, and the problem with limiting actions based on changed paths is that we can't do the same with the protected branch required checks. So if this action sometimes doesn't run, we can't require it.

[jcchavezs]

I don't think it is about the time it takes, it is mostly about this linter only makes sense if java code has been changed, hence the ignore.

[aaron-steinfeld]

please pin the env (we've been using ubuntu-20.04 elsewhere)

[aaron-steinfeld]

Is this to verify the check is working? The indentation should be 2 https://google.github.io/styleguide/javaguide.html#s4.2-block-indentation

[jcchavezs]

We need to fix all files before merge so next person opening a PR in here does not have to deal with the lint mess.

[aaron-steinfeld]

Agreed - but aren't these formatting changes going in the wrong direction? They're changing the formatting of files that were previously correctly formatted and after this change are incorrect. As an example, take Datastore.java - https://github.com/hypertrace/document-store/pull/24/files#diff-fcb7b4dcc9273e596438f61590e9eb81e373bec38fa40adec47e2068853fcd3a

The changes switch the block indent from 2 -> 4 (should be 2 https://google.github.io/styleguide/javaguide.html#s4.2-block-indentation ) and separates the lang imports from third party (should be in single block https://google.github.io/styleguide/javaguide.html#s3.3.3-import-ordering-and-spacing )

[jcchavezs]

Yeah, let me rephrase. This was an attempt to fix all problems but I was fooled by the intellij plugin I think.

[skjindal93]

Is there a way to auto format the code using some command line option?

I recently added https://github.com/diffplug/spotless in one of the repositories, which provides a command line option to auto format the entire code and it has various formatting options, google formatting being one of them https://github.com/diffplug/spotless/tree/main/plugin-gradle#google-java-format

We can add git hooks and auto apply/check the formatting as pre-push or pre-commit. Though, applying formatting on staged files is still an open issue in spotless diffplug/spotless#623, but we could still add a spotless check hook till then diffplug/spotless#178 (comment)

I think @pavolloffay also uses the same plugin in Java Agent https://github.com/hypertrace/javaagent/blob/a339bd5747af6cef4fbc666e641278b61cfbc273/gradle/spotless.gradle

[JBAhire]

Is there a way to auto format the code using some command line option?

I recently added https://github.com/diffplug/spotless in one of the repositories, which provides a command line option to auto format the entire code and it has various formatting options, google formatting being one of them https://github.com/diffplug/spotless/tree/main/plugin-gradle#google-java-format

We can add git hooks and auto apply/check the formatting as pre-push or pre-commit. Though, applying formatting on staged files is still an open issue in spotless diffplug/spotless#623, but we could still add a spotless check hook till then diffplug/spotless#178 (comment)

I think @pavolloffay also uses the same plugin in Java Agent https://github.com/hypertrace/javaagent/blob/a339bd5747af6cef4fbc666e641278b61cfbc273/gradle/spotless.gradle

@skjindal93, are you talking about something like this: https://github.com/thegreystone/action-jmc-spotless ?

Not sure if this helps. If not we can still write shellscript and use it with dockerfile and create our own action similar to above.

I believe it will be series of bash commands so we can have basic dockerfile with gradle as in above action and then add bash script as entrypoint.sh. would that help?

We will also need to figure out how GH actions can make a commit if it's going to make changes. Not sure about that.

Will be happy to work with you if this is significant and can help us in future as well. we can take a stab.

[skjindal93]

Is there a way to auto format the code using some command line option?
I recently added https://github.com/diffplug/spotless in one of the repositories, which provides a command line option to auto format the entire code and it has various formatting options, google formatting being one of them https://github.com/diffplug/spotless/tree/main/plugin-gradle#google-java-format
We can add git hooks and auto apply/check the formatting as pre-push or pre-commit. Though, applying formatting on staged files is still an open issue in spotless diffplug/spotless#623, but we could still add a spotless check hook till then diffplug/spotless#178 (comment)
I think @pavolloffay also uses the same plugin in Java Agent https://github.com/hypertrace/javaagent/blob/a339bd5747af6cef4fbc666e641278b61cfbc273/gradle/spotless.gradle

@skjindal93, are you talking about something like this: https://github.com/thegreystone/action-jmc-spotless ?

Not sure if this helps. If not we can still write shellscript and use it with dockerfile and create our own action similar to above.

I believe it will be series of bash commands so we can have basic dockerfile with gradle as in above action and then add bash script as entrypoint.sh. would that help?

We will also need to figure out how GH actions can make a commit if it's going to make changes. Not sure about that.

Will be happy to work with you if this is significant and can help us in future as well. we can take a stab.

If we just add spotless plugin in the list of applied plugins, spotless exposes a gradle command, which can be used as a separate Github Action as ./gradlew spotlessCheck. Or, you can make ./gradlew check depend on ./gradlew spotlessCheck and since ./gradlew build already runs ./gradlew check, it would be automatically taken care of in the CI pipeline

[jcchavezs]

I think the gradle plugin to autofix the code is better so we don't add another tool to fix the files. Maybe @pavolloffay can port his work into this repo?

[aaron-steinfeld]

Agree it would be nice to embed this into the existing check lifecycle so there's no ci changes needed and it can also be easily run locally as a standalone gradle task. I'm not sure how spotless works, but one other comment - personally I'd rather CI only fail/succeed based on the result of the check rather than actually change and commit code itself. Certainly nice to have an autofix option available to devs though.

[jcchavezs]

Agree it would be nice to embed this into the existing check lifecycle so there's no ci changes needed and it can also be easily run locally as a standalone gradle task. I'm not sure how spotless works, but one other comment - personally I'd rather CI only fail/succeed based on the result of the check rather than actually change and commit code itself. Certainly nice to have an autofix option available to devs though.

Check is good. I think it is too much overhead to only check diffs or diff files. This is linting and checking all files might be just fine. I am also against doing the fix on PR, this should be just a yes/no check and not do anything else. Autofixing on CI is cumbersome experience as 1. actions run in paralell and 2. we need push access 3. users have to download new commits before adding further changes.

[pavolloffay]

I think @pavolloffay also uses the same plugin in Java Agent https://github.com/hypertrace/javaagent/blob/a339bd5747af6cef4fbc666e641278b61cfbc273/gradle/spotless.gradle

Yes the HT javaagent uses the spotless gradle plugin and it works perfectly ❤️ . ./gradlew spotlessApply formats the code (one can also setup pre-commit hook to run it automagically, I am skipping automagic things:)). The build then checks if the code is formatted correctly - it is done during the unit test execution e.g. ./gradlew check so no separate CI job is needed.

[skjindal93]

@pavolloffay

one can also setup pre-commit hook to run it automagically, I am skipping automagic things

This one is still an open issue diffplug/spotless#623, unless we write a fancy bash script to do that