#274: Add input fields for maven coordinates for project creation #278
Conversation
…dation, generation and caching
|
Can one of the admins verify this patch? |
|
Any chance to get this reviewed? |
|
Hey, probably in a couple of weeks. At the moment, I am tied up with the Cargo Tracker Jakarta EE 10 upgrade. |
| public void onJakartaVersionChange() { | ||
| LOGGER.log(Level.INFO, |
There was a problem hiding this comment.
Please restore these log entries. They are important to understand what is going on in the application. The application is still very new, there are intermittent stability issues, and we should understand what the UI usage patterns are.
| @@ -175,9 +196,6 @@ public void onJakartaVersionChange() { | |||
| } | |||
|
|
|||
| public void onProfileChange() { | |||
| LOGGER.log(Level.INFO, | |||
There was a problem hiding this comment.
Please restore these log entries. They are important to understand what is going on in the application. The application is still very new, there are intermittent stability issues, and we should understand what the UI usage patterns are.
| @@ -209,9 +227,6 @@ public void onProfileChange() { | |||
| } | |||
|
|
|||
| public void onJavaVersionChange() { | |||
| LOGGER.log(Level.INFO, | |||
There was a problem hiding this comment.
Please restore these log entries. They are important to understand what is going on in the application. The application is still very new, there are intermittent stability issues, and we should understand what the UI usage patterns are.
| @@ -223,9 +238,6 @@ public void onJavaVersionChange() { | |||
| } | |||
|
|
|||
| public void onDockerChange() { | |||
| LOGGER.log(Level.INFO, | |||
There was a problem hiding this comment.
Please restore these log entries. They are important to understand what is going on in the application. The application is still very new, there are intermittent stability issues, and we should understand what the UI usage patterns are.
| @@ -239,9 +251,6 @@ public void onDockerChange() { | |||
| } | |||
|
|
|||
| public void onRuntimeChange() { | |||
| LOGGER.log(Level.INFO, | |||
There was a problem hiding this comment.
Please restore these log entries. They are important to understand what is going on in the application. The application is still very new, there are intermittent stability issues, and we should understand what the UI usage patterns are.
|
|
||
| MavenUtility.invokeMavenArchetype("org.eclipse.starter", "jakarta-starter", VersionInfo.ARCHETYPE_VERSION, | ||
| properties, workingDirectory); | ||
|
|
||
| LOGGER.info("Creating zip file."); | ||
| ZipUtility.zipDirectory(new File(workingDirectory, "jakartaee-hello-world"), workingDirectory); |
There was a problem hiding this comment.
This is not as simple a change as it may appear. In order to fully make this change, we really need to make quite a few changes across the project to really use the artifact ID as a file/directory/application name throughout. For now, I suggest keeping the zip file name the same as the application/generated war file name. An alternative is a much larger PR that uses artifact ID throughout (in fact I would say this is really the right thing to do if we want to go in this direction). This is one reason I am not so keen on this set of changes right now. I think we have other more pressing priorities.
There was a problem hiding this comment.
Hi @m-reza-rahman, at last I am getting the chance to look into this.
I don't quite understand the problem - all I did was changing the fixed string "jakartaee-hello-world" file name which a user downloads by what he inputed as artifactID. So why should that be a problem?
In general, I think it is currently bad not having a groupID and artifactID, so that the user has to manually change this after each generation, so even if we are not yet having the perfect solution, it should be much better than not having this.
There was a problem hiding this comment.
The problem is that it creates a further usability hazard by highlighting the fact that in the initial versions we really did not prioritize fully implementing the artifact ID and group ID, including at the Archetype level. So if we go this route now, let’s kindly implement it all the way.
Otherwise we are opening up a situation where we will need to implement it anyway, just at a more inconvenient time. A more comprehensive approach avoids this by simply taking the time up front to do it thoroughly in the first place instead of piecemeal - which I don’t know that I have the bandwidth for. I’d rather just review a comprehensive PR once.
| @@ -330,7 +347,10 @@ public void generate() { | |||
| } | |||
|
|
|||
| private String getCacheKey() { | |||
| return jakartaVersion + ":" + profile + ":" + javaVersion + ":" + docker + ":" + runtime; | |||
| // if groupid or artifactid was changed, don't use cache | |||
There was a problem hiding this comment.
Please handle this where the caching logic is. It's too abstract here.
|
Partly addressed the issues mentioned by Reza. The remaining one will hopefully be handled soon. |
|
Ok, then let's go for the full package. |
|
Great. I don’t think there’s a huge hurry. |
Feature implemented as described in #274
Tests done:
empty input fields
invalid input fields
input with and without leaving input field
generation with defaults
generation with new values
check for java packages
check for maven file
(Note: 2nd try after problems with email in commits)