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-342 - SDK fails if distro.properties supplies type or group for c… #302

Merged
merged 2 commits into from
Nov 8, 2024
Merged

SDK-342 - SDK fails if distro.properties supplies type or group for c… #302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
127a8d3
SDK-342 - SDK fails if distro.properties supplies type or group for c…
mseaton Nov 8, 2024
f91946c
Merge branch 'master' into SDK-342
mseaton Nov 8, 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
2 changes: 2 additions & 0 deletions integration-tests/src/test/resources/integration-test/openmrs-distro.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,6 @@ spa.frontendModules.@openmrs/esm-login-app=5.6.0
spa.frontendModules.@openmrs/esm-patient-chart-app=8.0.0
spa.apiUrl=notopenmrs
spa.configUrls=foo
content.hiv.groupId=org.openmrs.content
content.hiv.type=zip
content.hiv=1.0.0
13 changes: 9 additions & 4 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 @@ -154,9 +154,14 @@ public List<Artifact> getContentArtifacts() {
List<Artifact> artifacts = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

@mseaton wouldn't it be better to consolidate duplicated code for extracting different artifact types into a single reusable method?

Suggested change
List<Artifact> artifacts = new ArrayList<>();
public List<Artifact> getWarArtifacts(){
return getArtifacts(TYPE_WAR);
}
public List<Artifact> getConfigArtifacts() {
return getArtifacts(TYPE_CONFIG);
}
public List<Artifact> getContentArtifacts() {
return getArtifacts(TYPE_CONTENT);
}
private List<Artifact> getArtifacts(String typeContent) {
List<Artifact> artifacts = new ArrayList<>();
for (Object keyObject : getAllKeys()) {
String key = keyObject.toString();
String artifactType = getArtifactType(key);
if (artifactType.equals(typeContent)) {
artifacts.add(new Artifact(
checkIfOverwritten(key, ARTIFACT_ID),
getParam(key),
checkIfOverwritten(key, GROUP_ID),
checkIfOverwritten(key, TYPE)
));
}
}
return artifacts;

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed @wikumChamith there are many, many opportunities to work on tech debt in the SDK. This initial PR is attempting to just fix the underlying issue. More than happy to make broader refactoring improvements in other commits / tickets.

for (Object keyObject : getAllKeys()) {
String key = keyObject.toString();
if (key.startsWith(TYPE_CONTENT + ".")) {
artifacts.add(new Artifact(checkIfOverwritten(key, ARTIFACT_ID), getParam(key),
checkIfOverwritten(key, GROUP_ID), checkIfOverwritten(key, TYPE)));
String artifactType = getArtifactType(key);
if (artifactType.equals(TYPE_CONTENT)) {
artifacts.add(new Artifact(
checkIfOverwritten(key, ARTIFACT_ID),
getParam(key),
checkIfOverwritten(key, GROUP_ID),
checkIfOverwritten(key, TYPE)
));
}
}
return artifacts;
Expand All @@ -166,7 +171,7 @@ protected Set<Object> getAllKeys() {
return properties.keySet();
}

protected String getArtifactType(String key){
protected String getArtifactType(String key) {
String[] wordsArray = key.split("\\.");
if(!(wordsArray[wordsArray.length-1].equals(TYPE) || wordsArray[wordsArray.length-1].equals(ARTIFACT_ID) || wordsArray[wordsArray.length-1].equals(GROUP_ID))){
if(key.contains(".")){
Expand Down
15 changes: 4 additions & 11 deletions sdk-commons/src/main/java/org/openmrs/maven/plugins/utility/DistroHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,23 +641,16 @@ public void parseContentProperties(DistroProperties distroProperties) throws Moj
*/
public void downloadContentPackages(File contentPackageZipFile, DistroProperties distroProperties)
throws MojoExecutionException {
Properties contentProperties = new Properties();

for (Object key : distroProperties.getAllKeys()) {
String keyOb = key.toString();
if (!keyOb.startsWith(CONTENT_PREFIX)) {
continue;
}
Properties contentProperties = new Properties();

Artifact artifact = new Artifact(distroProperties.checkIfOverwritten(keyOb.replace(CONTENT_PREFIX, ""), ARTIFACT_ID), distroProperties.getParam(keyOb),
distroProperties.checkIfOverwritten(keyOb, GROUP_ID), distroProperties.checkIfOverwritten(keyOb, TYPE));
for (Artifact artifact : distroProperties.getContentArtifacts()) {

String version = distroProperties.get(keyOb);
String zipFileName = keyOb.replace(CONTENT_PREFIX, "") + "-" + version + ".zip";
String zipFileName = artifact.getArtifactId() + "-" + artifact.getVersion() + ".zip";
File zipFile = downloadDistro(contentPackageZipFile, artifact, zipFileName);

if (zipFile == null) {
log.warn("ZIP file not found for content package: {}", keyOb);
log.warn("ZIP file not found for content package: {}", artifact.getArtifactId());
continue;
}

Expand Down
38 changes: 38 additions & 0 deletions sdk-commons/src/test/java/org/openmrs/maven/plugins/model/DistroPropertiesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
import java.util.List;
import java.util.Properties;

import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.notNullValue;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.hasSize;

Expand Down Expand Up @@ -50,6 +52,42 @@ public void resolvePlaceholders_shouldReplaceVersions() throws MojoExecutionExce
assertThat(distro.getPlatformVersion(), is("222"));
}

@Test
public void shouldGetContentPropertiesWithDefaults() {
Properties properties = new Properties();
properties.setProperty("content.hiv", "1.0.0");
properties.setProperty("content.tb", "2.3.4-SNAPSHOT");
DistroProperties distro = new DistroProperties(properties);
List<Artifact> contentArtifacts = distro.getContentArtifacts();
assertThat(contentArtifacts, hasSize(2));
Artifact hiv = findArtifactByArtifactId(contentArtifacts, "hiv");
assertThat(hiv, notNullValue());
assertThat(hiv, hasVersion("1.0.0"));
assertThat(hiv.getGroupId(), equalTo(Artifact.GROUP_CONTENT));
assertThat(hiv.getType(), equalTo(Artifact.TYPE_ZIP));
Artifact tb = findArtifactByArtifactId(contentArtifacts, "tb");
assertThat(tb, notNullValue());
assertThat(tb, hasVersion("2.3.4-SNAPSHOT"));
assertThat(tb.getGroupId(), equalTo(Artifact.GROUP_CONTENT));
assertThat(tb.getType(), equalTo(Artifact.TYPE_ZIP));
}

@Test
public void shouldGetContentPropertiesWithOverrides() {
Properties properties = new Properties();
properties.setProperty("content.hiv", "1.0.0");
properties.setProperty("content.hiv.groupId", "org.openmrs.new");
properties.setProperty("content.hiv.type", "gzip");
DistroProperties distro = new DistroProperties(properties);
List<Artifact> contentArtifacts = distro.getContentArtifacts();
assertThat(contentArtifacts, hasSize(1));
Artifact hiv = findArtifactByArtifactId(contentArtifacts, "hiv");
assertThat(hiv, notNullValue());
assertThat(hiv, hasVersion("1.0.0"));
assertThat(hiv.getGroupId(), equalTo("org.openmrs.new"));
assertThat(hiv.getType(), equalTo("gzip"));
}

@Test(expected = MojoExecutionException.class)
public void resolvePlaceholders_shouldFailIfNoPropertyForPlaceholderFound() throws MojoExecutionException {
Properties properties = new Properties();
Expand Down
Loading