search

eclipse-ee4j / starter

Eclipse Starter for Jakarta EE
Eclipse Public License 2.0
50 stars 38 forks source link

Support for GlassFish Docker in the archetype #326

Open OndroMih opened 1 month ago

OndroMih commented 1 month ago

This adds support for GlassFish Docker into the archetype:

 mvn archetype:generate -DarchetypeGroupId=org.eclipse.starter -DarchetypeArtifactId=jakarta-starter -DarchetypeVersion=2.4.0-SNAPSHOT -Druntime=glassfish -Ddocker=yes -Dprofile=full -DjakartaVersion=10 -DjavaVersion=17

Only for the above parameters (EE10, Java 17, Full profile).

If this PR is merged, I plan to add support for GlassFish Docker in the UI and also add tests that cover other runtimes.

m-reza-rahman commented 1 month ago

Could you kindly try to minimize the white space only changes? It is really difficult to see what the actual changes are and it also pollutes the change log. If they are really necessary, please create a separate PR for the desired white space changes?

Once that is done, I would also like to understand why so many new files are needed for a seemingly small set of new features.

OndroMih commented 1 month ago

The big amount of files is for the tests. The tests verify that the generated project contains expected files. Maybe the tests are not needed as the Github workflow covers some of the cases and uses the appropriate Java version. I just used them to run tests locally.

eclipse-starter-bot commented 1 month ago

Can one of the admins verify this patch?

m-reza-rahman commented 1 month ago

OK. Can you please enhance the nightly build if required: https://github.com/eclipse-ee4j/starter/blob/master/.github/workflows/nightly.yml? It should provide reasonable test coverage.

OndroMih commented 1 month ago

I removed the tests in maven build and added some tests to the main and nightly builds.

m-reza-rahman commented 1 month ago

OK. Eventually we can look into moving the tests into the Maven build if there is a reasonable way.

Please do address the extraneous white space changes before I can review for a merge. Do note, there is a reason the white space is the way it is. It avoids users encountering strange formatting as Velocity does not ignore white spaces in the output, even when placed in the same line as conditionals.

OndroMih commented 1 month ago

I removed all the unnecessary extraneous white space changes.

I kept the extra spaces in velocity templates to indent blocks and added maven antrun plugin that removes them during the build. Without indenting velocity templates, it's hard to understand them. If you want, I can submit this change as a separate PR. Or even before this PR, as it would be easier to review my changes once the velocity templates are indented.

m-reza-rahman commented 1 month ago

Yes, please make that a separate PR to keep the change log clean. I’ll also need to ensure there are no unintended side effects. Since that’s not a functional change, it can be done after the functional change.

OndroMih commented 1 month ago

I created a separate PR only with the indentation changes here: https://github.com/eclipse-ee4j/starter/pull/327

Until that one is merged, I changed this PR to draft.