-
Notifications
You must be signed in to change notification settings - Fork 2
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
base: master
Are you sure you want to change the base?
Conversation
It is deprecated and has been read-only for some time.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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. ;-)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!?
There was a problem hiding this comment.
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()}"
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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. ;-)
There was a problem hiding this comment.
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 Report
@@ 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.
|
There was a problem hiding this 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()}"
This means the information can be shared among tasks. E.g. jar and bootJar can both use it.
Done. What is missing is testing with Java 16/17 as stated in the other review/thread. |
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.