Pull and extract the content package set in distro.prop #286

nravilla · 2024-08-22T03:26:22Z

Description of what I changed

Modified classes to download and extract and move content packages (just the backend content)

Issue I worked on

https://openmrs.atlassian.net/browse/O3-3640

Checklist: I completed these to help reviewers :)

My IDE is configured to follow the code style of this project.
Acceptance tests (setup and build-distro was tested and it works
I ran mvn clean install right before creating this pull request and
added all formatting changes to my commit.
All existing tests passed.
My pull request is based on the latest changes of the master branch.

mherman22

Thanks @nravilla a few comments as we wait for @ibacher's review

mherman22 · 2024-08-23T05:43:59Z

sdk-commons/src/main/java/org/openmrs/maven/plugins/model/BaseSdkProperties.java

+				System.out.println(artifactId);
+				System.out.println(getParam(key));
+				System.out.println(Artifact.GROUP_CONTENT);
+


i think we can remove these System.out.printlns during clean up after all the reviews

mherman22 · 2024-08-23T05:46:13Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/utility/ModuleInstaller.java

was this change intended?

ibacher

Thanks @nravilla. Because of the number of formatting changes, this is a really hard PR to review. I've left a few pointers on things that stuck out to me, but I'll need more time to review the remaining files in any detail.

ibacher · 2024-08-23T14:54:41Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/utility/ContentHelper.java

+		this.pluginManager = pluginManager;
+	}
+
+	public void downloadContent(File configurationDir, File tempContentDir, Artifact contentArtifact,


Suggested change

public void downloadContent(File configurationDir, File tempContentDir, Artifact contentArtifact,

public void downloadContent(File configurationDir, File contentDir, Artifact contentArtifact,

ibacher · 2024-08-23T14:55:51Z

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

+		File targetDirectory = new File(configDir, "temp_content");
+		targetDirectory.mkdir();
+
+		List<Artifact> contents = distroProperties.getContentArtifacts(distroHelper, targetDirectory);
+		if (!contents.isEmpty()) {
+			wizard.showMessage("Downloading Contents...\n");
+			for (Artifact content : contents) {
+				wizard.showMessage("Downloading Content: " + content);
+				contentHelper.downloadContent(configDir, targetDirectory, content, moduleInstaller);
+			}
+		}
+		FileUtils.deleteQuietly(targetDirectory);


Seems like this should be wrapped in something like:

Suggested change

File targetDirectory = new File(configDir, "temp_content");

targetDirectory.mkdir();

List<Artifact> contents = distroProperties.getContentArtifacts(distroHelper, targetDirectory);

if (!contents.isEmpty()) {

wizard.showMessage("Downloading Contents...\n");

for (Artifact content : contents) {

wizard.showMessage("Downloading Content: " + content);

contentHelper.downloadContent(configDir, targetDirectory, content, moduleInstaller);

}

}

FileUtils.deleteQuietly(targetDirectory);

File targetDirectory = new File(configDir, "temp_content");

targetDirectory.mkdir();

try {

List<Artifact> contents = distroProperties.getContentArtifacts(distroHelper, targetDirectory);

if (!contents.isEmpty()) {

wizard.showMessage("Downloading Contents...\n");

for (Artifact content : contents) {

wizard.showMessage("Downloading Content: " + content);

contentHelper.downloadContent(configDir, targetDirectory, content, moduleInstaller);

}

}

} finally {

FileUtils.deleteQuietly(targetDirectory);

}

Also, I always think that we should use Files.createTempDirectory() to create a temporary directory.

ibacher · 2024-08-23T14:58:32Z

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

+
+		List<Artifact> contents = distroProperties.getContentArtifacts(distroHelper, targetDirectory);
+		if (!contents.isEmpty()) {
+			wizard.showMessage("Downloading Contents...\n");


Suggested change

wizard.showMessage("Downloading Contents...\n");

wizard.showMessage("Downloading content packages...\n");

ibacher · 2024-08-23T14:58:44Z

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

+		if (!contents.isEmpty()) {
+			wizard.showMessage("Downloading Contents...\n");
+			for (Artifact content : contents) {
+				wizard.showMessage("Downloading Content: " + content);


Suggested change

wizard.showMessage("Downloading Content: " + content);

wizard.showMessage("Downloading content package: " + content);

ibacher · 2024-08-23T15:00:25Z

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

+
+	private void installContents(Server server, DistroProperties distroProperties) throws MojoExecutionException {
+		if (distroProperties != null) {
+			File tempContentDir = new File(server.getServerDirectory(), "temp-content");


Same comments here as before

ibacher · 2024-08-23T15:03:57Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/utility/ContentHelper.java

+		if (backendConfigs == null || backendConfigs.listFiles().length == 0) {
+			throw new MojoExecutionException("No directories to process under content configuration directories");
+		}


There is no way that backendConfigs could be null here.

ibacher · 2024-08-23T15:05:44Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/utility/ContentHelper.java

+			throw new MojoExecutionException("No directories to process under content configuration directories");
+		}
+
+		for (File backendConfig : backendConfigs.listFiles()) {


Suggested change

for (File backendConfig : backendConfigs.listFiles()) {

for (File configDir : backendConfigs.listFiles()) {

ibacher · 2024-08-23T15:08:32Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/utility/ContentHelper.java

+			if (!matchingConfigurationDir.exists() && !matchingConfigurationDir.isDirectory()) {
+				//this folder is from content zip
+				artifactDir = matchingConfigurationDir;
+			} else {
+				// Create a new folder with the artifactId under the matching output directory
+				artifactDir = new File(matchingConfigurationDir, artifactId);
+				if (!artifactDir.exists()) {
+					artifactDir.mkdirs();
+				}
+			}


Am I reading the logic here correctly? It says "if matchingConfigurationDir does not exist and it is not a directory, set artifactDir to matchingConfigurationDir, otherwise create a new subfolder"?

Assuming I'm reading that correctly:

Under what circumstance can the if branch be true?

We always want to ensure that we are writing to a subfolder of the configuration directory, right?

ibacher · 2024-08-23T15:09:21Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/utility/ContentHelper.java

+			}
+
+			// Copy the content from inputDir to the newly created artifactDir
+			copyDirectory(backendConfig, artifactDir);


FileUtils.copyDirectory() can be used here.

ibacher · 2024-08-23T15:10:04Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/utility/ModuleInstaller.java

-                executionEnvironment(mavenProject, mavenSession, pluginManager)
-        );
-    }
+


I don't love all the reformatting in this PR. It makes it very hard to review and know if anything has changed and if so, what.

Please don't commit files you haven't touched other than reformatting. If there are any changes here, can you please use GitHub comments to indicate what those are?

This reverts commit 384e7b3.

nravilla · 2024-08-26T21:17:04Z

@ibacher @mherman22 Unable to resolve the formatting issues, will close this and create a new branch

suruchee requested review from rbuisson and ibacher August 22, 2024 03:47

mherman22 reviewed Aug 23, 2024

View reviewed changes

ibacher requested changes Aug 23, 2024

View reviewed changes

Pull and extract the content package set in distro.prop

384e7b3

nravilla force-pushed the O3-3640 branch from 8454640 to 384e7b3 Compare August 26, 2024 16:51

Ravilla and others added 3 commits August 26, 2024 13:51

Revert "Pull and extract the content package set in distro.prop"

5476188

This reverts commit 384e7b3.

Handles content property

d056e13

Handles content packages

c7f53cd

nravilla closed this Aug 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pull and extract the content package set in distro.prop #286

Pull and extract the content package set in distro.prop #286

nravilla commented Aug 22, 2024 •

edited by mherman22

Loading

mherman22 left a comment

mherman22 Aug 23, 2024

mherman22 Aug 23, 2024

ibacher left a comment

ibacher Aug 23, 2024

ibacher Aug 23, 2024

ibacher Aug 23, 2024

ibacher Aug 23, 2024

ibacher Aug 23, 2024

ibacher Aug 23, 2024

ibacher Aug 23, 2024

ibacher Aug 23, 2024

ibacher Aug 23, 2024

ibacher Aug 23, 2024

ibacher Aug 23, 2024

ibacher Aug 23, 2024

nravilla commented Aug 26, 2024

	public void downloadContent(File configurationDir, File tempContentDir, Artifact contentArtifact,
	public void downloadContent(File configurationDir, File contentDir, Artifact contentArtifact,

	wizard.showMessage("Downloading Contents...\n");
	wizard.showMessage("Downloading content packages...\n");

	wizard.showMessage("Downloading Content: " + content);
	wizard.showMessage("Downloading content package: " + content);

	for (File backendConfig : backendConfigs.listFiles()) {
	for (File configDir : backendConfigs.listFiles()) {

Pull and extract the content package set in distro.prop #286

Pull and extract the content package set in distro.prop #286

Conversation

nravilla commented Aug 22, 2024 • edited by mherman22 Loading

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

mherman22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

nravilla commented Aug 26, 2024

nravilla commented Aug 22, 2024 •

edited by mherman22

Loading