O3-3776: Ensure sdk generates a content package template project #284

mherman22 · 2024-08-19T14:16:49Z

Description of what I changed

Issue I worked on

see https://openmrs.atlassian.net/browse/O3-3776

Checklist: I completed these to help reviewers :)

My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
I ran mvn clean install right before creating this pull request and
added all formatting changes to my commit.

No? -> execute the above command
All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

ibacher

Nice work! A few improvements to make.

I think that ideally in the test here, we'd have a reference. See here.

ibacher · 2024-08-20T14:35:14Z

archetype-module-content/pom.xml

+      <extension>
+        <groupId>org.apache.maven.archetype</groupId>
+        <artifactId>archetype-packaging</artifactId>
+        <version>2.2</version>


Let's update to the newest version (3.2.1)

ibacher · 2024-08-20T14:35:38Z

archetype-module-content/src/main/resources/META-INF/maven/archetype-metadata.xml

+        <requiredProperty key="moduleName" />
+        <requiredProperty key="moduleDescription"/>
+        <requiredProperty key="moduleAuthor">
+            <defaultValue>mherman22</defaultValue>


Heh... I don't think you want this?

archetype-module-content/src/main/resources/META-INF/maven/archetype-metadata.xml

ibacher · 2024-08-20T14:38:41Z

archetype-module-content/src/main/resources/META-INF/maven/archetype-metadata.xml

+                      xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
+    <requiredProperties>
+        <requiredProperty key="groupId">
+            <defaultValue>org.openmrs.module</defaultValue>


Suggested change

<defaultValue>org.openmrs.module</defaultValue>

<defaultValue>org.openmrs.content</defaultValue>

ibacher · 2024-08-20T14:39:04Z

archetype-module-content/src/main/resources/META-INF/maven/archetype-metadata.xml

+        <requiredProperty key="moduleAuthor">
+            <defaultValue>mherman22</defaultValue>
+        </requiredProperty>
+        <requiredProperty key="moduleClassnamePrefix" />


We don't need this for content packages, I don't think.

True that! I will add a check that skips adding that property for content-package type.

ibacher · 2024-08-20T14:40:52Z

archetype-module-content/src/main/resources/archetype-resources/assembly.xml

+            </includes>
+        </fileSet>
+    </fileSets>
+</assembly>


Suggested change

</assembly>

</assembly>

ibacher · 2024-08-20T14:41:39Z

archetype-module-content/src/main/resources/archetype-resources/pom.xml

+    <groupId>org.openmrs.content</groupId>
+    <artifactId>content-package</artifactId>
+    <version>1.0.0-SNAPSHOT</version>
+    <name></name>


Suggested change

<name></name>

<name>${moduleName}</name>

ibacher · 2024-08-20T14:42:23Z

archetype-module-content/src/main/resources/archetype-resources/pom.xml

+    <groupId>org.openmrs.content</groupId>
+    <artifactId>content-package</artifactId>
+    <version>1.0.0-SNAPSHOT</version>


Suggested change

<groupId>org.openmrs.content</groupId>

<artifactId>content-package</artifactId>

<version>1.0.0-SNAPSHOT</version>

<groupId>${groupId}</groupId>

<artifactId>${artifactId}</artifactId>

<version>${version}</version>

ibacher · 2024-08-20T14:43:13Z

archetype-module-content/src/main/resources/archetype-resources/pom.xml

+    <scm>
+        <connection>scm:git:[email protected]:openmrs/openmrs-content-hiv.git</connection>
+        <developerConnection>scm:git:[email protected]:openmrs/openmrs-content-hiv.git</developerConnection>
+        <url>https://github.com/openmrs/openmrs-content-hiv.git</url>
+    </scm>


Suggested change

<scm>

<connection>scm:git:[email protected]:openmrs/openmrs-content-hiv.git</connection>

<developerConnection>scm:git:[email protected]:openmrs/openmrs-content-hiv.git</developerConnection>

<url>https://github.com/openmrs/openmrs-content-hiv.git</url>

</scm>

<scm>

<connection>scm:git:[email protected]:openmrs/openmrs-content-${artifactId}.git</connection>

<developerConnection>scm:git:[email protected]:openmrs/openmrs-content-${artifactId}.git</developerConnection>

<url>https://github.com/openmrs/openmrs-content-${artifactId}.git</url>

</scm>

ibacher · 2024-08-20T14:44:13Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/CreateProject.java

@@ -269,6 +281,10 @@ private void createModule() throws MojoExecutionException {
 					"What is the lowest version of the Reference Application (-D%s) you want to support?", refapp, "refapp",
 					"2.4");
 			archetypeArtifactId = SDKConstants.REFAPP_ARCH_ARTIFACT_ID;
+		} else if (TYPE_CONTENT_PACKAGE.equals(type)) {
+			contentpackage = wizard.promptForValueIfMissingWithDefault("What is the lowest version of the content package (-D%s) you want to support?", contentpackage, "contentpackage",


I don't really understand what this question is prompting for?

It didn't make sense to me either. Was just going by the convention overthere, I can skip the prompt.

Thanks

Oh, yeah. For other things we care about the platform version, etc. We currently don't for content packages, but maybe we should?

ibacher · 2024-08-23T15:34:29Z

Does this need a re-review?

mherman22 · 2024-08-23T15:37:30Z

Does this need a re-review?

Looks ok to me

ibacher · 2024-08-26T12:33:17Z

archetype-module-content/src/test/resources/projects/basic/archetype.properties

+artifactId=content-package
+openmrsContentPackageVersion=1.0.0
+package=org.openmrs.content.content-package
+version=${package.getClass().forName("java.lang.String").getConstructor($package.getClass().forName("java.lang.String")).newInstance("1.0.0-SNAPSHOT")}


We don't need the weird expression here.

Suggested change

version=${package.getClass().forName("java.lang.String").getConstructor($package.getClass().forName("java.lang.String")).newInstance("1.0.0-SNAPSHOT")}

version=1.0.0-SNAPSHOT

Is this required to get the test to pass or something?

Ensure sdk generates a content package template project

b597860

mherman22 requested a review from ibacher August 19, 2024 15:41

ibacher requested changes Aug 20, 2024

View reviewed changes

mherman22 added 6 commits August 21, 2024 10:20

changes according to reviews

0be7951

add fix

caeb109

add reference project and mvn goal in goal.txt

85eac8f

remove .gitignore since its failing the integration tests

a3ed8c2

add expression as default value for version

a6120c7

fix

88c5a46

mherman22 requested a review from ibacher August 22, 2024 14:49

ibacher reviewed Aug 26, 2024

View reviewed changes

remove

1f0fdc9

ibacher approved these changes Aug 26, 2024

View reviewed changes

ibacher merged commit 58f4b90 into openmrs:master Aug 26, 2024
2 checks passed

mherman22 deleted the O3-3776 branch August 26, 2024 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

O3-3776: Ensure sdk generates a content package template project #284

O3-3776: Ensure sdk generates a content package template project #284

mherman22 commented Aug 19, 2024 •

edited

Loading

ibacher left a comment

ibacher Aug 20, 2024

ibacher Aug 20, 2024

ibacher Aug 20, 2024

ibacher Aug 20, 2024

mherman22 Aug 20, 2024

ibacher Aug 20, 2024

ibacher Aug 20, 2024

ibacher Aug 20, 2024

ibacher Aug 20, 2024

ibacher Aug 20, 2024

mherman22 Aug 20, 2024

ibacher Aug 20, 2024

ibacher commented Aug 23, 2024

mherman22 commented Aug 23, 2024

ibacher Aug 26, 2024

ibacher Aug 26, 2024

	<defaultValue>org.openmrs.module</defaultValue>
	<defaultValue>org.openmrs.content</defaultValue>

	version=${package.getClass().forName("java.lang.String").getConstructor($package.getClass().forName("java.lang.String")).newInstance("1.0.0-SNAPSHOT")}
	version=1.0.0-SNAPSHOT

O3-3776: Ensure sdk generates a content package template project #284

O3-3776: Ensure sdk generates a content package template project #284

Conversation

mherman22 commented Aug 19, 2024 • edited Loading

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

ibacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibacher commented Aug 23, 2024

mherman22 commented Aug 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mherman22 commented Aug 19, 2024 •

edited

Loading