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.

Sign up for GitHub

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

SDK-354 - Fixing bugs and adding test cases for the AddExclusions goal #306

Merged
merged 6 commits into from
Nov 15, 2024
Merged

SDK-354 - Fixing bugs and adding test cases for the AddExclusions goal #306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
934cc98
SDK-354 - Fix
mseaton Nov 15, 2024
07e7dc5
Undo one change no longer needed
mseaton Nov 15, 2024
f793477
Fix
mseaton Nov 15, 2024
4a182b5
Unit tests for normalize
mseaton Nov 15, 2024
1ed8e29
Unit tests for normalize
mseaton Nov 15, 2024
cbf22a8
Unit tests for parent distro
mseaton Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 70 additions & 9 deletions integration-tests/src/test/java/org/openmrs/maven/plugins/AddExclusionIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,88 @@
package org.openmrs.maven.plugins;

import org.apache.commons.io.FileUtils;
import org.junit.Test;
import org.openmrs.maven.plugins.model.DistroProperties;
import org.openmrs.maven.plugins.utility.DistroHelper;

import java.io.File;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;

public class AddExclusionIntegrationTest extends AbstractSdkIntegrationTest {

public DistroProperties getDistroProperties() {
return DistroHelper.getDistroPropertiesFromFile(distroFile);
}

@Test
public void shouldAddExclusionIfNoParentDefined() throws Exception {
includeDistroPropertiesFile("openmrs-distro.properties");
assertNotNull(getDistroProperties());
assertNull(getDistroProperties().getParentDistroArtifact());
assertTrue(getDistroProperties().getExclusions().isEmpty());
executeExclusionTask(distroFile, "omod.uicommons");
assertNotNull(getDistroProperties());
assertTrue(getDistroProperties().getExclusions().contains("omod.uicommons"));
assertTrue(getLogContents().contains(AddExclusion.WARNING_NO_PARENT_DISTRO));
}

@Test
public void shouldAddExclusion() throws Exception {
DistroProperties distroProperties = DistroHelper.getDistroPropertiesFromFile(distroFile);
assertNotNull(distroProperties);
assertFalse(distroProperties.getExclusions().contains("omod.uicommons"));
public void shouldAddExclusionForPropertyContainedInParent() throws Exception {
includeDistroPropertiesFile("openmrs-distro-parent-as-parent.properties");
assertNotNull(getDistroProperties());
assertNotNull(getDistroProperties().getParentDistroArtifact());
assertTrue(getDistroProperties().getExclusions().isEmpty());
executeExclusionTask(distroFile, "omod.uicommons");
assertNotNull(getDistroProperties());
assertTrue(getDistroProperties().getExclusions().contains("omod.uicommons"));
assertFalse(getLogContents().contains(AddExclusion.WARNING_NO_PARENT_DISTRO));
assertFalse(getLogContents().contains(AddExclusion.WARNING_PROPERTY_NOT_IN_PARENT));
}

addTaskParam("distro", distroFile.getAbsolutePath());
addTaskParam("property", "omod.uicommons");
executeTask("exclusion");
@Test
public void shouldAddExclusionForPropertyContainedInDistro() throws Exception {
includeDistroPropertiesFile("openmrs-distro-parent-as-distro.properties");
assertNotNull(getDistroProperties());
assertNotNull(getDistroProperties().getParentDistroArtifact());
assertTrue(getDistroProperties().getExclusions().isEmpty());
executeExclusionTask(distroFile, "omod.uicommons");
assertNotNull(getDistroProperties());
assertTrue(getDistroProperties().getExclusions().contains("omod.uicommons"));
assertFalse(getLogContents().contains(AddExclusion.WARNING_NO_PARENT_DISTRO));
assertFalse(getLogContents().contains(AddExclusion.WARNING_PROPERTY_NOT_IN_PARENT));
}

@Test
public void shouldAddExclusionForPropertyNotContainedInParent() throws Exception {
includeDistroPropertiesFile("openmrs-distro-parent-as-parent.properties");
assertNotNull(getDistroProperties());
assertNotNull(getDistroProperties().getParentDistroArtifact());
assertTrue(getDistroProperties().getExclusions().isEmpty());
executeExclusionTask(distroFile, "omod.invalidmodulename");
assertNotNull(getDistroProperties());
assertTrue(getDistroProperties().getExclusions().contains("omod.invalidmodulename"));
assertFalse(getLogContents().contains(AddExclusion.WARNING_NO_PARENT_DISTRO));
assertTrue(getLogContents().contains(AddExclusion.WARNING_PROPERTY_NOT_IN_PARENT));
}

private String getLogContents() throws Exception {
File logFile = new File(testDirectory, "log.txt");
assertTrue(logFile.exists());
return FileUtils.readFileToString(logFile, "UTF-8");
}

private void executeExclusionTask(File distroFile, String exclusion) throws Exception {
if (distroFile != null) {
addTaskParam("distro", distroFile.getAbsolutePath());
}
if (exclusion != null) {
addTaskParam("property", exclusion);
}
executeTask("exclusion");
assertSuccess();
distroProperties = DistroHelper.getDistroPropertiesFromFile(distroFile);
assertTrue(distroProperties.getExclusions().contains("omod.uicommons"));
}
}
6 changes: 3 additions & 3 deletions integration-tests/src/test/java/org/openmrs/maven/plugins/SetupIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ public void setup_shouldInstallServerWithGivenJavaHomeAndAddJavaHomeToSdkPropert

@Test
public void setup_shouldInstallWithParentDistroSpecifiedInDistroProperties() throws Exception {
includeDistroPropertiesFile("openmrs-distro-parent-distro.properties");
includeDistroPropertiesFile("openmrs-distro-parent-as-distro.properties");
addTaskParam("distro", testDirectory.toString() + File.separator + "openmrs-distro.properties");
addMockDbSettings();

Expand All @@ -362,9 +362,9 @@ public void setup_shouldInstallWithParentDistroSpecifiedInDistroProperties() thr

executeTask("setup");

assertFilePresent( serverId, "openmrs-2.0.1.war");
assertFilePresent( serverId, "openmrs-2.0.5.war");
assertModulesInstalled(serverId, "htmlformentry-3.3.1.omod");
assertFileNotPresent(serverId, "modules", "htmlformentry-3.3.0.omod");
assertFileNotPresent(serverId, "modules", "htmlformentry-3.10.0.omod");
}

@Test
Expand Down
5 changes: 5 additions & 0 deletions ...tion-tests/src/test/resources/integration-test/openmrs-distro-parent-as-distro.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
name=Spa Artifact Example
version=2.5
distro.referenceapplication=2.6.1
omod.htmlformentry=3.3.1
db.h2.supported=false
8 changes: 8 additions & 0 deletions ...tion-tests/src/test/resources/integration-test/openmrs-distro-parent-as-parent.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
name=Spa Artifact Example
version=2.5
parent.groupId=org.openmrs.distro
parent.artifactId=referenceapplication-package
parent.version=2.6.1
parent.type=jar
omod.htmlformentry=3.3.1
db.h2.supported=false
7 changes: 0 additions & 7 deletions ...gration-tests/src/test/resources/integration-test/openmrs-distro-parent-distro.properties
View file
Open in desktop

This file was deleted.

4 changes: 0 additions & 4 deletions ...gration-tests/src/test/resources/integration-test/openmrs-distro-spa-artifacts.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
name=Spa Artifact Example
version=1.0.0-SNAPSHOT

war.openmrs=2.6.9

omod.spa=2.0.0

spa.artifactId=openmrs-frontend-zl
spa.groupId=org.pih.openmrs
spa.version=1.3.0
spa.includes=openmrs-frontend-zl-1.3.0

db.h2.supported=false
30 changes: 17 additions & 13 deletions maven-plugin/src/main/java/org/openmrs/maven/plugins/AddExclusion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
@Mojo(name = "exclusion", requiresProject = false)
public class AddExclusion extends AbstractTask {

public static final String WARNING_PROPERTY_NOT_IN_PARENT = "The property is not included in the parent distro";
public static final String WARNING_NO_PARENT_DISTRO = "This distro properties file does not contain a valid parent distro";

/**
* Path to the openmrs-distro.properties file to modify
*/
Expand Down Expand Up @@ -51,22 +54,23 @@ public void executeTask() throws MojoExecutionException, MojoFailureException {
throw new MojoFailureException("Invalid distro file");
}

Artifact distroArtifact = originalDistroProperties.getParentArtifact();
if (StringUtils.isNotBlank(distroArtifact.getArtifactId()) && StringUtils.isNotBlank(distroArtifact.getGroupId()) && StringUtils.isNotBlank(distroArtifact.getVersion())) {
DistroProperties distroProperties = distroHelper.resolveParentArtifact(distroArtifact, new File(distro).getParentFile(), originalDistroProperties, null);
Copy link
Member Author

Choose a reason for hiding this comment

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

In my testing, this was actually not getting the full set of parent properties. This code also does not resolve full ancestors (eg. if the parent has a parent). The change below intends to fix this.

Artifact parentDistro = originalDistroProperties.getParentDistroArtifact();
if (parentDistro != null) {
DistroProperties parentProperties = distroHelper.downloadDistroProperties(new File(distro).getParentFile(), parentDistro);
parentProperties = distroHelper.getDistroPropertiesForFullAncestry(parentProperties, new File(distro).getParentFile());
if (StringUtils.isBlank(property)) {
List<String> currentExclusions = distroProperties.getExclusions();
List<String> options = distroProperties.getPropertyNames().stream().filter(prop -> !currentExclusions.contains(prop)).collect(Collectors.toList());
property = wizard.promptForMissingValueWithOptions("Enter the property you want to exclude",
null, null, options);
} else {
if (!distroProperties.getPropertyNames().contains(property)) {
wizard.showWarning("The property is not included in the parent distro");
List<String> currentExclusions = originalDistroProperties.getExclusions();
List<String> options = parentProperties.getPropertyNames().stream().filter(prop -> !currentExclusions.contains(prop)).collect(Collectors.toList());
Copy link
Member Author

Choose a reason for hiding this comment

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

Notice the difference here from above - the options need to come from the parent properties, but the current exclusions need to come from the child properties

property = wizard.promptForMissingValueWithOptions("Enter the property you want to exclude", null, null, options);
}
else {
if (!parentProperties.getPropertyNames().contains(property)) {
wizard.showWarning(WARNING_PROPERTY_NOT_IN_PARENT);
}
}

} else {
wizard.showWarning("This distro properties file does not contain a valid parent distro");
}
else {
wizard.showWarning(WARNING_NO_PARENT_DISTRO);
if (StringUtils.isBlank(property)) {
property = wizard.promptForValueIfMissing(null, "property to exclude");
if (StringUtils.isBlank(property)) {
Expand Down
6 changes: 3 additions & 3 deletions maven-plugin/src/main/java/org/openmrs/maven/plugins/BuildDistro.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ public void executeTask() throws MojoExecutionException, MojoFailureException {
if (distroFile.exists()) {
wizard.showMessage("Building distribution from the distro file at " + distroFile + "...\n");
distroProperties = new DistroProperties(distroFile);
distroArtifact = distroProperties.getParentArtifact();
distroArtifact = distroProperties.getParentDistroArtifact();
distroProperties = handleParentDistroProperties(distroArtifact, buildDirectory, distroProperties);
} else if (Project.hasProject(userDir)) {
Project config = Project.loadProject(userDir);
Expand All @@ -156,7 +156,7 @@ public void executeTask() throws MojoExecutionException, MojoFailureException {
}
} else if (StringUtils.isNotBlank(distro)) {
distroProperties = distroHelper.resolveDistroPropertiesForStringSpecifier(distro, versionsHelper);
distroArtifact = distroProperties.getParentArtifact();
distroArtifact = distroProperties.getParentDistroArtifact();
distroProperties = handleParentDistroProperties(distroArtifact, buildDirectory, distroProperties);
}

Expand Down Expand Up @@ -201,7 +201,7 @@ public void executeTask() throws MojoExecutionException, MojoFailureException {
}

private DistroProperties handleParentDistroProperties(Artifact distroArtifact, File buildDirectory, DistroProperties distroProperties) throws MojoExecutionException {
if (StringUtils.isNotBlank(distroArtifact.getArtifactId()) && StringUtils.isNotBlank(distroArtifact.getGroupId()) && StringUtils.isNotBlank(distroArtifact.getVersion())) {
if (distroArtifact != null) {
distroProperties = distroHelper.resolveParentArtifact(distroArtifact, buildDirectory, distroProperties, appShellVersion);
}
return distroProperties;
Expand Down
4 changes: 2 additions & 2 deletions maven-plugin/src/main/java/org/openmrs/maven/plugins/Setup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,8 @@ private DistroProperties resolveDistroProperties(Server server) throws MojoExecu
break;

default: // distro properties from current directory
Artifact distroArtifact = distroProperties.getParentArtifact();
if (StringUtils.isNotBlank(distroArtifact.getArtifactId()) && StringUtils.isNotBlank(distroArtifact.getGroupId()) && StringUtils.isNotBlank(distroArtifact.getVersion())) {
Artifact distroArtifact = distroProperties.getParentDistroArtifact();
if (distroArtifact != null) {
distroProperties = distroHelper.resolveParentArtifact(distroArtifact, server, distroProperties, appShellVersion);
} else {
server.setPlatformVersion(
Expand Down
1 change: 1 addition & 0 deletions sdk-commons/src/main/java/org/openmrs/maven/plugins/model/BaseSdkProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ public abstract class BaseSdkProperties {
public static final String VERSION = "version";
public static final String TYPE_CONTENT = "content";
public static final String TYPE_DISTRO = "distro";
public static final String TYPE_PARENT = "parent"; // This is an alternative to "distro"
public static final String TYPE_OWA = "owa";
public static final String TYPE_SPA = "spa";
public static final String TYPE_CONFIG = "config";
Expand Down
77 changes: 45 additions & 32 deletions sdk-commons/src/main/java/org/openmrs/maven/plugins/model/DistroProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,46 +109,59 @@ private String extractPropertyName(String key) {
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

We can review this in more detail in SDK-348, but for now I have consolidated the two methods of specifying a parent distro so that both are equally supported. See change below.

public Artifact getDistroArtifact() {
/**
* Allowed formats for artifactId and version
* distro.artifactId=version
* distro.artifactId.groupId=
* distro.artifactId.type=
* -OR-
* distro.anything=version
* distro.anything.artifactId=
* distro.anything.groupId=
* distro.anything.type=
* -OR-
* parent.artifactId=
* parent.groupId=
* parent.version=
* parent.type=
*/
public Artifact getParentDistroArtifact() throws MojoExecutionException {
Artifact artifact = null;
for (Object keyObject: getAllKeys()) {
String key = keyObject.toString();
String artifactType = getArtifactType(key);
if(artifactType.equals(TYPE_DISTRO)) {
return new Artifact(checkIfOverwritten(key, ARTIFACT_ID), getParam(key), checkIfOverwritten(key, GROUP_ID), checkIfOverwritten(key, TYPE), "jar");
if (artifactType.equals(TYPE_DISTRO)) {
String artifactId = checkIfOverwritten(key, ARTIFACT_ID);
String version = getParam(key);
String groupId = checkIfOverwritten(key, GROUP_ID);
String type = checkIfOverwritten(key, TYPE);
if (artifact != null) {
throw new MojoExecutionException("Only a single " + TYPE_DISTRO + " property can be added to indicate the parent distribution");
}
artifact = new Artifact(artifactId, version, groupId, type);
}
}
return null;
}

public Artifact getParentArtifact() {
String parentArtifactId = getParam("parent.artifactId");
String parentGroupId = getParam("parent.groupId");
String parentVersion = getParam("parent.version");

int missingCount = 0;
if (StringUtils.isBlank(parentArtifactId)) {
missingCount++;
}
if (StringUtils.isBlank(parentGroupId)) {
missingCount++;
}
if (StringUtils.isBlank(parentVersion)) {
missingCount++;
}

// We are only going to throw an error if only one or two parameters are missing
if (missingCount > 0 && missingCount < 3) {
throw new IllegalArgumentException("Missing arguments for the parent");
String artifactId = getParam(TYPE_PARENT + "." + ARTIFACT_ID);
String version = getParam(TYPE_PARENT + "." + VERSION);
String groupId = getParam(TYPE_PARENT + "." + GROUP_ID);
String type = getParam(TYPE_PARENT + "." + TYPE, TYPE_ZIP);
if (StringUtils.isNotBlank(artifactId)) {
if (artifact != null) {
throw new MojoExecutionException("Distro properties cannot define both a " + TYPE_DISTRO + " and a " + TYPE_PARENT + " property");
}
if (StringUtils.isBlank(version) || StringUtils.isBlank(groupId)) {
throw new MojoExecutionException("You must specify a " + TYPE_PARENT + " groupId and version if you specify and artifactId");
}
artifact = new Artifact(artifactId, version, groupId, type);
}

return new Artifact(parentArtifactId, parentVersion, parentGroupId, "zip");
return DistroHelper.normalizeArtifact(artifact, null);
}

public List<Artifact> getModuleArtifacts(DistroHelper distroHelper, File directory) throws MojoExecutionException {
List<Artifact> childArtifacts = getModuleArtifacts();
List<Artifact> parentArtifacts = new ArrayList<>();

Artifact artifact = getDistroArtifact();
Artifact artifact = getParentDistroArtifact();
if (artifact != null) {
DistroProperties distroProperties = distroHelper.downloadDistroProperties(directory, artifact);
parentArtifacts.addAll(distroProperties.getModuleArtifacts(distroHelper, directory));
Expand All @@ -160,7 +173,7 @@ public List<Artifact> getOwaArtifacts(DistroHelper distroHelper, File directory)
List<Artifact> childArtifacts = getOwaArtifacts();
List<Artifact> parentArtifacts = new ArrayList<>();

Artifact artifact = getDistroArtifact();
Artifact artifact = getParentDistroArtifact();
if (artifact != null) {
DistroProperties distroProperties = distroHelper.downloadDistroProperties(directory, artifact);
parentArtifacts.addAll(distroProperties.getOwaArtifacts(distroHelper, directory));
Expand All @@ -171,7 +184,7 @@ public List<Artifact> getOwaArtifacts(DistroHelper distroHelper, File directory)
public Map<String, String> getSpaProperties(DistroHelper distroHelper, File directory) throws MojoExecutionException {
Map<String, String> spaProperties = getSpaProperties();

Artifact artifact = getDistroArtifact();
Artifact artifact = getParentDistroArtifact();
if (artifact != null) {
DistroProperties distroProperties = distroHelper.downloadDistroProperties(directory, artifact);
spaProperties.putAll(distroProperties.getSpaProperties(distroHelper, directory));
Expand All @@ -183,7 +196,7 @@ public List<Artifact> getWarArtifacts(DistroHelper distroHelper, File directory)
List<Artifact> childArtifacts = getWarArtifacts();
List<Artifact> parentArtifacts = new ArrayList<>();

Artifact artifact = getDistroArtifact();
Artifact artifact = getParentDistroArtifact();
if (artifact != null) {
DistroProperties distroProperties = distroHelper.downloadDistroProperties(directory, artifact);
parentArtifacts.addAll(distroProperties.getWarArtifacts(distroHelper, directory));
Expand All @@ -192,7 +205,7 @@ public List<Artifact> getWarArtifacts(DistroHelper distroHelper, File directory)
}

public String getPlatformVersion(DistroHelper distroHelper, File directory) throws MojoExecutionException{
Artifact artifact = getDistroArtifact();
Artifact artifact = getParentDistroArtifact();
if (artifact != null) {
DistroProperties distroProperties = distroHelper.downloadDistroProperties(directory, artifact);
return distroProperties.getPlatformVersion(distroHelper, directory);
Expand Down
Loading
Loading