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

Gradle scripts cleanup and minimal profile. #6

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

Pfeil
Copy link
Member

@Pfeil Pfeil commented Nov 16, 2021

As I fiddled around with the pit service gradle files anyway I decided to adjust the template in the way we talked about today. I restructured the files, updated the dependencies and plugins as high as possible with this gradle 6 version.

  1. There are quiet some special dependencies in there, e.g. javassist for byte code manipulation, javax.activation etc. Should we keep this as some kind of list of all our used dependencies, as weird as they might be? I think our internal wiki might be a better place for that. So what should we keep in here, what not?
  2. Should I update to Gradle 7? There will then be new deprecation warnings for the build scripts as a preparation to Gradle 8, but I fixed them for PIT already so I could do it. Some argument against it? Currently Gradle 6 only limits us to use a newer lombok plugin version (but not the lombok version, so it is kind of fine). So this is only a very small pro argument.
  3. Any ideas what to put into the minimal profile by default? I thought maybe a bootjar without the asciidoctor part, and the one with asciidoctor in default. I am not sure about splitting dependencies or plugins...
  4. I hope it is okay for you I used so many comments, but I find the build scripts pretty hard to read.
  5. As the defaults seem to be documented nowhere, I am not sure how minimal the minimal profile really is.

@Pfeil Pfeil marked this pull request as ready for review November 23, 2021 10:33
@Pfeil Pfeil mentioned this pull request Nov 24, 2021
Copy link
Contributor

@VolkerHartmann VolkerHartmann left a comment

Choose a reason for hiding this comment

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

I like your comments inside the gradle config files very much.
Nevertheless there are two small issues...

"version": jar.archiveVersion,
"date": new SimpleDateFormat("yyyy-MM-dd").format(new Date())
// Spring RESTdocs extension for Asciidoctor config
inputs.dir snippetsDir
Copy link
Contributor

Choose a reason for hiding this comment

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

This plugin fails if tests are skipped and build directory was cleaned in beforehand.
To avoid this take a look at https://github.com/kit-data-manager/metastore2/blob/master/build.gradle line 168.
You may skip the warning but in my case parts of the documentation are generated during tests and are part of the jar-file and therefore tests are mandatory.
On the other hand. You don't need a snippetsDir if you don't use it. ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

To be honest, I am not fully aware of the usefulness of this directory. I would say that, if every service should use it at some point, let us use it (by default) in the template. If not I'll comment it out (even as a dependency then). But I guess the choice should be made by someone who knows about its usefulness.

Copy link
Member Author

@Pfeil Pfeil Nov 24, 2021

Choose a reason for hiding this comment

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

Also, ./gradlew clean asciidoctor does not fail for me. What do I have to do to reproduce the problem? Are you sure it is not fixed by dependsOn test? I think I basically followed official documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Try ./gradlew clean -x test clean build to reproduce this error.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want an online documentation inside the jar containing automatically created snippets how to deal with the REST interface it might be a good idea to use this feature. In case of static documentation it's not necessary at all.
See: https://github.com/kit-data-manager/metastore2/blob/master/restDocu.md#getting-a-metadata-schema-record
The snippets shown here are generated during the tests. (In this case I also commited this as a static documentation to avoid building and executing metastore2 to see how it works.)
I like this automatically generated snippets as they are always up to date even when there are changes in the API. In the case of static documentation, the documentation often no longer corresponds to the current status. This may be very frustrating for the users relying on the docu.

Copy link
Contributor

Choose a reason for hiding this comment

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

OOOPS, you're right. Ok, 'depends on test' makes the trick I think. So everything seems to be fine. ASCII doc will be only generated when tests are enabled!
Just one remark. I tried it with JDK16 which will not work!?

Copy link
Contributor

Choose a reason for hiding this comment

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

Another remark you should print some information during the build script:
println "Running gradle version: $gradle.gradleVersion"
println "Building ${name} version: ${version}"
println "JDK version: ${JavaVersion.current()}"

Copy link
Member Author

Choose a reason for hiding this comment

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

I now do print this information, but I am unsure about the current Java version. It can be misleading, as I think this is the Java Version running Gradle, not necessarily the one building the application if someone sets a toolchain, I think: https://docs.gradle.org/current/userguide/toolchains.html#sec:consuming

Might be related: options.release in compileJava

I can not test openjdk 16 that easily as brew on osx only seems to have the LTS versions and the newest one (which is already 17).

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we agree on focussing on LTS versions? I'm not sure if we have the capacity to jump on every new JDK-train immediately. ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Tests with different versions are part of the gitHub action so you won't have to test all versions in detail.
Regarding toolchain: If the java version is overwritten manually you may be aware of this. In that case you may also change the message accordingly.

@codecov-commenter
Copy link

codecov-commenter commented Nov 24, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@5fb7387). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master       #6   +/-   ##
=========================================
  Coverage          ?   60.00%           
  Complexity        ?        2           
=========================================
  Files             ?        1           
  Lines             ?        5           
  Branches          ?        0           
=========================================
  Hits              ?        3           
  Misses            ?        2           
  Partials          ?        0           

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 5fb7387...acf36a7. Read the comment docs.

@Pfeil Pfeil mentioned this pull request Nov 24, 2021
Copy link
Contributor

@VolkerHartmann VolkerHartmann left a comment

Choose a reason for hiding this comment

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

As this template could be used by projects in the future the current LTS version of Java should be supported. Due to this the project should rely on

  • gradle 7.3
  • Spring Boot 2.6.0

If there are any problems during build some additional infos at command line would be
helpful:
println "Building '${name}' version: ${version}"
println "Running gradle version: $gradle.gradleVersion"
println "JDK version: ${JavaVersion.current()}"

@Pfeil
Copy link
Member Author

Pfeil commented Nov 30, 2021

Done. What is missing is testing with Java 16/17 as stated in the other review/thread.

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.

4 participants