SDK-354 - Fixing bugs and adding test cases for the AddExclusions goal #306
Conversation
| @@ -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); | |||
There was a problem hiding this comment.
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.
| 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()); |
There was a problem hiding this comment.
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
| @@ -109,46 +109,59 @@ private String extractPropertyName(String key) { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
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.
| if (version.equalsIgnoreCase(SDKConstants.LATEST_SNAPSHOT_BATCH_KEYWORD)) { | ||
| version = versionsHelper.getLatestSnapshotVersion(new Artifact(artifactId, version, groupId)); | ||
| } else if (version.equalsIgnoreCase((SDKConstants.LATEST_VERSION_BATCH_KEYWORD))) { | ||
| version = versionsHelper.getLatestReleasedVersion(new Artifact(artifactId, version, groupId)); |
There was a problem hiding this comment.
There is a lot of scattered code in the SDK that does "special purpose stuff". One example of that is resolving "referenceapplication" to either "referenceapplication-package" or "referenceapplication-distro" depending on the version. This was not being applied everywhere equally, so this is an effort to create a method for doing this that can be called from various places to gain consistency. More refactoring and consolidation down the road will help, but this is a start.
| } else { | ||
| return PropertiesUtils.getFrontendPropertiesFromSpaConfigUrl( | ||
| "https://raw.githubusercontent.com/openmrs/openmrs-distro-referenceapplication/" + artifact.getVersion() + "/frontend/spa-build-config.json"); | ||
| if (artifact.getArtifactId().equals("referenceapplication-distro")) { |
There was a problem hiding this comment.
This doesn't work for all cases. Specifically, I was testing with a refapp 2.x distro as the parent, and in trying to do that it was failing here, as no resource in github exists for spa-build-config.json in a 2.x release/tag of the refapp, as far as I can tell.
So I wrapped this in a check to only resolve these properties if we are dealing with the "referenceapplication-distro" artifact, which is what is used starting in refapp 3.x. Can you confirm this is what we want @ibacher and @dkayiwa ? This whole downloading frontend config from github is...odd.
| @@ -555,6 +579,38 @@ public Properties getArtifactProperties(Artifact artifact, File directory, Strin | |||
| return properties; | |||
| } | |||
|
|
|||
| // TODO: This needs unit tests, but to do so will require more refactoring to allow downloadDistroProperties to be mocked. | |||
There was a problem hiding this comment.
Per my comment, this needs to have proper tests around it, but I'm punting that to a later date due to the potential complexity of doing this in a unit test and needing to mock methods within the class being tested.
see https://openmrs.atlassian.net/browse/SDK-354