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

Support for GlassFish Docker in the archetype #326

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

Conversation

OndroMih
Copy link
Contributor

@OndroMih OndroMih commented Aug 14, 2024

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).

  • first commit adds the functionality
  • second commit adds tests into the Github workflow

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

@OndroMih OndroMih force-pushed the ondromih-glassfish-docker branch from a49de69 to 0b60c39 Compare August 14, 2024 00:55
@m-reza-rahman
Copy link
Contributor

m-reza-rahman commented Aug 14, 2024

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 OndroMih force-pushed the ondromih-glassfish-docker branch 3 times, most recently from e0fa55f to ba0685a Compare August 14, 2024 01:53
@m-reza-rahman m-reza-rahman self-assigned this Aug 14, 2024
@OndroMih OndroMih force-pushed the ondromih-glassfish-docker branch from ba0685a to 148ea44 Compare August 14, 2024 01:55
@OndroMih
Copy link
Contributor Author

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.

@OndroMih OndroMih force-pushed the ondromih-glassfish-docker branch from 148ea44 to 40c0093 Compare August 14, 2024 02:18
@eclipse-starter-bot
Copy link

Can one of the admins verify this patch?

@OndroMih OndroMih force-pushed the ondromih-glassfish-docker branch 4 times, most recently from 016ed52 to 9e57615 Compare August 14, 2024 03:15
@m-reza-rahman
Copy link
Contributor

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 OndroMih force-pushed the ondromih-glassfish-docker branch from 9e57615 to 704ff8f Compare August 14, 2024 04:11
@OndroMih
Copy link
Contributor Author

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

@OndroMih OndroMih force-pushed the ondromih-glassfish-docker branch 2 times, most recently from fe8f1c5 to 0613184 Compare August 14, 2024 04:19
@m-reza-rahman
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

m-reza-rahman commented Aug 15, 2024

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 OndroMih marked this pull request as draft August 15, 2024 23:09
@OndroMih
Copy link
Contributor Author

OndroMih commented Aug 15, 2024

I created a separate PR only with the indentation changes here: #327

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

@OndroMih OndroMih force-pushed the ondromih-glassfish-docker branch from 0613184 to 1be40c1 Compare January 22, 2025 01:30
@OndroMih OndroMih marked this pull request as ready for review January 22, 2025 01:34
@OndroMih
Copy link
Contributor Author

I rebased on top of the current master, which already contains the Velocity indentation. So this PR contains only changes related to GlassFish and Docker.

@OndroMih OndroMih force-pushed the ondromih-glassfish-docker branch from 1be40c1 to 3d4a3ee Compare January 22, 2025 02:22
@m-reza-rahman
Copy link
Contributor

OK. I plan to look at this once the EE 11 support is well in hand.

# Conflicts:
#	.github/workflows/nightly.yml
@hantsy
Copy link

hantsy commented Feb 18, 2025

Great to include this as soon as possible.

@m-reza-rahman
Copy link
Contributor

Hi @OndroMih, my plan is to look at this next weekend. Can you kindly do me a favor and try to sync with the latest code (ideally both UI and Archetype)?

@OndroMih
Copy link
Contributor Author

@m-reza-rahman, done.

There are only archetype changes in this PR. UI changes will follow in another PR.

When I tried to do it initially in this PR, I couldn't figure how to do it in the UI properly, I always broken something. I'd like to refactor the UI so that it's easier to modify without breaking some rules. I'd like to do these things step by step, rather in a single PR, so that it's easier to review.

@m-reza-rahman
Copy link
Contributor

m-reza-rahman commented Feb 23, 2025

OK. If it’s more than trying to simplify the validation logic (which is more than welcome), I ask that we kindly discuss the changes before necessary making a PR. The current functionality is based on project consensus, but obviously nothing is set in stone.

You can always leave the UI changes to me for this time. You could review my changes for future reference too. It’s really just a matter of thinking through the validation cases, at least for me.

@OndroMih
Copy link
Contributor Author

OndroMih commented Feb 23, 2025 via email

@m-reza-rahman
Copy link
Contributor

m-reza-rahman commented Feb 23, 2025

OK, no problem. I'll handle the UI part. Maybe I could go over the changes with you once they are done? There is no denying the validation logic is involved. I would love to see what could be done to improve it and also see if other folks could do the work when needed. However, this is definitely a lower priority than simply getting things done.

@OndroMih
Copy link
Contributor Author

Currently, I have no concrete idea how to improve the validation logic. I only wasn't able to amend it for GlassFish in Docker correctly, and that blocked me from updating the UI.

From my experience, it's better to use declarative rules, and refresh each component to update according to the rules and the state. So that the rules need to be changed only in a single place. The current solution with change listeners, which update the model according to a recent change in the form, is really hard to maintain, and requires changes in all the change listeners.

I would suggest that there's a single method recomputeOptions, that would simply recompute all the available options based on the current selections in the form. This method would be called on each change, and would update all the selector components. The controller (JSF) updates the chosen options, which would trigger recomputation of the available options and render the view. The recomputeOptions method would be easy to test. If it works correctly, it would guarantee that the form would match the rules in all cases, because the form would just reflect the recomputed data.

@m-reza-rahman
Copy link
Contributor

OK. An idea worth pursuing. Let me give it some thought once things slow down a bit.

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