O3-3511: Content package to create zip file #1

nravilla · 2024-07-18T20:35:01Z

Requirements
https://openmrs.atlassian.net/browse/O3-3511

Summary

Updated maven pom.xml to run ContentPropertiesValidator and to create assembly if content.properties are valid.
Created ContentPropertiesValidator, ContentPropertiesValidatorTest, and assembly.xml

To validate and create assembly, run : mvn clean package

alaboso · 2024-07-25T07:06:41Z

There is a conflict with pom.xml. @nravilla could you please check it out.

nravilla · 2024-07-25T11:44:35Z

There is a conflict with pom.xml. @nravilla could you please check it out.

Sure Amos, there's been some changes, the java code is moved to another location. will update and resubmit this PR. thank you for checking this out

pom.xml

ibacher

Thanks @nravilla! I've written up a few small comments on this. Also note that we have conventions for how PRs should appear, i.e., they must reference a ticket, the message should be written in "sentence casing", etc.

ibacher · 2024-07-30T19:09:11Z

assembly.xml

+      </includes>
+    </fileSet>
+    <fileSet>
+      <directory>${project.basedir}/configs</directory>     


Suggested change

<directory>${project.basedir}/configs</directory>

<directory>${project.basedir}/configs</directory>

is there a change suggested here?

Removing whitespace

configs/frontend_config/config.json

ibacher · 2024-07-30T19:10:37Z

content.properties

+spa.frontendModules.@openmrs/esm-login-app=^3.0.0
+spa.frontendModules.@openmrs/esm-patient-chart=^1.0.0


Although this is good for demo purposes, I don't think we need this here.

ibacher · 2024-07-30T19:10:42Z

content.properties

+# content.properties - Metadata for the content package and its dependencies
+
+# The name of this content package
+name=openmrs-content-hiv


Suggested change

name=openmrs-content-hiv

name=hiv

Ideally, however, this would be populated from the POM directly:

Suggested change

name=openmrs-content-hiv

name=${project.artifactId}

The above recommended change has been implemented, I don't know much about the data in content.properties, Amos will be responsible for the data. i just have name and version for the time being.

content.properties

ibacher · 2024-07-30T19:13:59Z

pom.xml

+			<plugin>
+				<groupId>org.apache.maven.plugins</groupId>
+				<artifactId>maven-assembly-plugin</artifactId>
+				<version>3.3.0</version>


We should always use the latest version available, if possible.

Suggested change

<version>3.3.0</version>

<version>3.7.1</version>

ibacher · 2024-07-30T19:14:57Z

pom.xml

+							<appendAssemblyId>false</appendAssemblyId>
+							<!-- Ensure this is set to false -->


It'd likely be nice to have a why for this comment, but at least it should go with the right part.

Suggested change

<appendAssemblyId>false</appendAssemblyId>





<appendAssemblyId>false</appendAssemblyId>

Have added a comment, at the top instead of bottom.

ibacher · 2024-07-30T19:16:14Z

pom.xml

+	<modelVersion>4.0.0</modelVersion>
+ 	<groupId>org.openmrs.content</groupId>
+  	<artifactId>openmrs-content-hiv</artifactId>
+  	<version>1.0-SNAPSHOT</version>


As discussed:

Suggested change

<version>1.0-SNAPSHOT</version>

<version>1.0.0-SNAPSHOT</version>

ibacher · 2024-07-30T19:17:12Z

pom.xml

+						<goals>
+						<goal>validate-assemble</goal>
+						</goals>


Suggested change

<goals>

<goal>validate-assemble</goal>

</goals>

<goals>

<goal>validate-assemble</goal>

</goals>

ibacher · 2024-07-30T19:17:19Z

pom.xml

+				<executions>
+					<execution>
+						<id>validate-assemble</id>
+						<phase>package</phase>						


Suggested change

<phase>package</phase>

<phase>package</phase>

I also think this should probably be run in the verify phase rather than the package phase. Technically, that comes after the Zip is generated, but in production usage, we're doing to be running the deploy phase to publish this, and that will block that.

done! updated it to verify phase instead of package phase.

ibacher · 2024-08-08T14:36:55Z

Could you add the ticket number to the PR title and fix the merge conflicts please? This should be ready to go from there.

gracepotma · 2024-08-13T19:06:13Z

Update from @nravilla by email: "I've updated the pom.xml to fetch the 1.8.0 packager-plugin that I released this morning. I also added another execution for validate-configuration as requested, but it's failing. Should we remove it, or can you fix it? It's unrelated to the new plugin I introduced. For now, could you comment out the validate-configuration execution and merge my changes into the main branch? This will complete my task."

gracepotma · 2024-08-14T01:38:40Z

Hey @alaboso & @wikumChamith - it would be great if you could help out with reviewing this, if you are available? (Well, re-reviewing, in Amos' case :) ) This PR is turning the repo into the model for future content packages, so it's important to get the whole setup correct - and Ian is very over-booked this week.

wikumChamith · 2024-08-14T09:27:40Z

@nravilla can we get the build errors fixed?

nravilla · 2024-08-14T17:35:48Z

@nravilla can we get the build errors fixed?

Hi Wikum, Ian requested adding another verify step (validate-configurations) and it’s unrelated to my current work. If I comment out that step, the build works fine, please see Grace's message above (email from me)

As asked, to fix build errors, I am commenting that step out for the time being, if Ian really needs it, we can address it as another PR.

ibacher · 2024-08-14T17:44:09Z

@nravilla That step needs to work in CI for the work here to be complete. I realize it's not something you built or changed, but it is validating that the configuration in the package is complete and self-contained which is a required property of content packages.

nravilla · 2024-08-14T18:15:07Z

@nravilla That step needs to work in CI for the work here to be complete. I realize it's not something you built or changed, but it is validating that the configuration in the package is complete and self-contained which is a required property of content packages.

@ibacher, had added that step earlier and i see some issues with it in CI, that's the reason i proposed we deal with it as a separate PR, please see the log at - https://ci.openmrs.org/browse/CH-CON-RTM-4/log

rbuisson · 2024-08-15T12:14:24Z

It would be best to skip validation of the actual configuration files for now so to unblock Bamboo and publish the package as is.
In the mean time, @alaboso should be able to fix the configurations so that the OpenMRS validation passes.

ibacher · 2024-08-15T12:15:43Z

@rbuisson It's less configurations that are broken and more the validator itself... I guess we just by-pass this for now.

I've been trying to play around with it, but I can't get the validator to run on my ARM-based Mac, I don't really have a good solution.

validates content.properties and creates a zip file

6bfbdf3

Changed conf directory structure

9e06b21

rbuisson reviewed Jul 29, 2024

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

pom.xml Outdated Show resolved Hide resolved

ibacher requested changes Jul 30, 2024

View reviewed changes

Formatting and PR review changes

81aacee

nravilla requested a review from ibacher August 8, 2024 14:18

Ravilla and others added 5 commits August 12, 2024 10:59

Changes from PR review

01e636a

Update pom.xml, Added validate-configurations

02de783

Update pom.xml, added distributionManagement

8e95a0f

Added pluginRepospluginRepositories section

aa83015

Changed sourceDir for validate-configurations

3d2a838

gracepotma changed the title ~~validates content.properties and creates a zip file~~ O3-3511: validates content.properties and creates a zip file Aug 13, 2024

gracepotma requested review from alaboso and wikumChamith August 13, 2024 19:09

Update pom.xml, removed validate-configurations

f1d883c

Ravilla and others added 6 commits August 14, 2024 23:41

Changed http to https

7464118

Updated urls

993a936

Added profiles to control execution

e52fb11

Update pom.xml, changed sourceFile to sourceDir

8002868

Added profiles to control execution

bb1d6eb

Formatting update

83e6df5

ibacher approved these changes Aug 15, 2024

View reviewed changes

ibacher changed the title ~~O3-3511: validates content.properties and creates a zip file~~ O3-3511: Content package to create zip file Aug 15, 2024

ibacher merged commit ebe07b4 into openmrs:main Aug 15, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

O3-3511: Content package to create zip file #1

O3-3511: Content package to create zip file #1

nravilla commented Jul 18, 2024 •

edited

Loading

alaboso commented Jul 25, 2024

nravilla commented Jul 25, 2024

ibacher left a comment

ibacher Jul 30, 2024

nravilla Jul 31, 2024

ibacher Aug 1, 2024

ibacher Jul 30, 2024

ibacher Jul 30, 2024

ibacher Jul 30, 2024

nravilla Aug 8, 2024

ibacher Jul 30, 2024

nravilla Aug 8, 2024

ibacher Jul 30, 2024

nravilla Aug 8, 2024

ibacher Jul 30, 2024

ibacher Jul 30, 2024

ibacher Jul 30, 2024

ibacher Jul 30, 2024

nravilla Aug 8, 2024

ibacher commented Aug 8, 2024

gracepotma commented Aug 13, 2024

gracepotma commented Aug 14, 2024

wikumChamith commented Aug 14, 2024

nravilla commented Aug 14, 2024 •

edited

Loading

ibacher commented Aug 14, 2024

nravilla commented Aug 14, 2024

rbuisson commented Aug 15, 2024

ibacher commented Aug 15, 2024 •

edited

Loading

	<directory>${project.basedir}/configs</directory>
	<directory>${project.basedir}/configs</directory>

		spa.frontendModules.@openmrs/esm-login-app=^3.0.0
		spa.frontendModules.@openmrs/esm-patient-chart=^1.0.0

		<appendAssemblyId>false</appendAssemblyId>
		<!-- Ensure this is set to false -->

	<version>1.0-SNAPSHOT</version>
	<version>1.0.0-SNAPSHOT</version>

O3-3511: Content package to create zip file #1

O3-3511: Content package to create zip file #1

Conversation

nravilla commented Jul 18, 2024 • edited Loading

alaboso commented Jul 25, 2024

nravilla commented Jul 25, 2024

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

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 8, 2024

gracepotma commented Aug 13, 2024

gracepotma commented Aug 14, 2024

wikumChamith commented Aug 14, 2024

nravilla commented Aug 14, 2024 • edited Loading

ibacher commented Aug 14, 2024

nravilla commented Aug 14, 2024

rbuisson commented Aug 15, 2024

ibacher commented Aug 15, 2024 • edited Loading

nravilla commented Jul 18, 2024 •

edited

Loading

nravilla commented Aug 14, 2024 •

edited

Loading

ibacher commented Aug 15, 2024 •

edited

Loading