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

Add all positive cases for None and WildFly runtimes #307

Merged
merged 6 commits into from
Jul 1, 2024
Merged

Add all positive cases for None and WildFly runtimes #307

merged 6 commits into from
Jul 1, 2024

Conversation

Tasnim1147
Copy link
Contributor

No description provided.

@eclipse-starter-bot
Copy link

Can one of the admins verify this patch?

Copy link
Contributor

@m-reza-rahman m-reza-rahman left a comment

Choose a reason for hiding this comment

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

Please address all review feedback.

@m-reza-rahman m-reza-rahman marked this pull request as draft June 24, 2024 18:36
@m-reza-rahman
Copy link
Contributor

m-reza-rahman commented Jun 24, 2024

Is this once again ready for review? When it is, please take status out of draft.

@m-reza-rahman m-reza-rahman changed the title Add all possitive cases for None and WildFly runtimes Add all positive cases for None and WildFly runtimes Jun 24, 2024
@Tasnim1147 Tasnim1147 marked this pull request as ready for review June 27, 2024 13:34
Copy link
Contributor

@m-reza-rahman m-reza-rahman left a comment

Choose a reason for hiding this comment

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

Please order by SE version (starting with the lowest), runtime/none (alphabetic, starting with none), EE version (starting with the lowest), profile (full, web, core), and Docker (no, yes).

So like this:

  • Run Archetype for EE 8 Web Profile, SE 8, Payara
  • Run Archetype for EE 8 Web Profile, SE 8, Payara, with Docker

Where Docker is really not supported (a warning is generated by the Archetype), we should not treat that as a "positive case". That's a negative case we need to test for later.

Please order your part and the file overall - including existing cases. Confusing to read and evaluate otherwise for me.

@m-reza-rahman m-reza-rahman marked this pull request as draft June 27, 2024 23:39
@Tasnim1147 Tasnim1147 marked this pull request as ready for review June 29, 2024 03:24
@Tasnim1147 Tasnim1147 marked this pull request as draft June 29, 2024 03:27
@Tasnim1147 Tasnim1147 requested a review from m-reza-rahman June 29, 2024 03:29
@Tasnim1147 Tasnim1147 marked this pull request as ready for review June 29, 2024 03:29
@@ -30,82 +30,437 @@ jobs:
run: |
mvn archetype:generate -DinteractiveMode=false -DaskForDefaultPropertyValues=false -DarchetypeGroupId=org.eclipse.starter -DarchetypeArtifactId=jakarta-starter -DarchetypeVersion=2.3.0-SNAPSHOT -DjakartaVersion=8 -Dprofile=full -DjavaVersion=8 -DoutputDirectory=app -Dgoals="clean package"
rm -rf app


- name: Run Archetype for EE 8 Full Profile, SE 8
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a duplicate of the one above. Please delete one.

Copy link
Contributor

@m-reza-rahman m-reza-rahman left a comment

Choose a reason for hiding this comment

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

Please re-read and understand the previous review comments. All cases where Docker is not a valid option should be removed as it is not really a "positive" case. These will be added later as negative cases where we explicitly check that a warning was generated and there is no Dockerfile.

@m-reza-rahman m-reza-rahman marked this pull request as draft June 30, 2024 18:33
@m-reza-rahman
Copy link
Contributor

Is this ready for another review?

@Tasnim1147
Copy link
Contributor Author

Tasnim1147 commented Jul 1, 2024

I apologize for my oversight. I initially thought I was generating only the positive cases based on the UI, but upon further inspection, I realized this was not the case. Thank you for pointing this out.

As for duplicate cases, I kept the original ones. I have also changed the naming scheme to match the original cases. However, for the following duplicate case I kept the new one.

Original

     - name: Run Archetype for EE 10 Web Profile, SE 17, WildFly
       run: |
          mvn archetype:generate -DinteractiveMode=false -DaskForDefaultPropertyValues=false -DarchetypeGroupId=org.eclipse.starter -DarchetypeArtifactId=jakarta-starter -DarchetypeVersion=2.3.0-SNAPSHOT -DjakartaVersion=10 -Dprofile=web -DjavaVersion=17 -Druntime=wildfly -Ddocker=yes -DoutputDirectory=app/wildfly -Dgoals="clean package"
          rm -rf app/wildfly

New

     - name: Run Archetype for EE 10 Web Profile, SE 17, WildFly, with Docker
       run: |
          mvn archetype:generate -DinteractiveMode=false -DaskForDefaultPropertyValues=false -DarchetypeGroupId=org.eclipse.starter -DarchetypeArtifactId=jakarta-starter -DarchetypeVersion=2.3.0-SNAPSHOT -DjakartaVersion=10 -Dprofile=web -DjavaVersion=17 -Druntime=wildfly -Ddocker=yes -DoutputDirectory=app/wildfly -Dgoals="clean package"
          rm -rf app/wildfly

@Tasnim1147 Tasnim1147 marked this pull request as ready for review July 1, 2024 03:41
@m-reza-rahman
Copy link
Contributor

NP. That’s the point of the review. Hopefully I can take a look during the work week.

@m-reza-rahman
Copy link
Contributor

Looks great! Thanks for working through this.

@m-reza-rahman m-reza-rahman merged commit 00094e6 into eclipse-ee4j:master Jul 1, 2024
1 check passed
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