O3-3433: SDK Command: Add dependencies to distro.properties #283

wikumChamith · 2024-08-19T12:16:03Z

Description of what I changed

This adds a plugin that allows users to manipulate a distro.properties file by adding dependencies. This supports adding OMOD, SPA, OWA, and WAR dependencies.

Issue I worked on

see https://openmrs.atlassian.net/browse/O3-3433

Checklist: I completed these to help reviewers :)

My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
I ran mvn clean install right before creating this pull request and
added all formatting changes to my commit.

No? -> execute the above command
All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

ibacher

Thanks @wikumChamith! Just a few things to adjust... Not sure how to handle the two PRs introducing similar functionality issue...

ibacher · 2024-08-20T17:31:30Z

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

+            }
+        }
+
+        if(StringUtils.isBlank(type)) {


Shouldn't we also prompt for this in the case that the type doesn't match one of the known types?

ibacher · 2024-08-20T17:31:44Z

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

+            properties = distroHelper.resolveDistroPropertiesForStringSpecifier(distro, versionsHelper).getProperties();
+        }
+
+        switch (type) {


Suggested change

switch (type) {

switch (type.toUpperCase()) {

ibacher · 2024-08-20T17:32:57Z

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

+                    version = wizard.promptForMissingValueWithOptions("Enter the module version", null, null, versions,
+                            "Please specify the SPA version", null);
+                }
+                properties.setProperty("spa.frontendModules.@openmrs/" + moduleName, version);


The @openmrs/ part is a part of the module name (and we need to allow for implementations that can't publish to our NPM org)

Suggested change

properties.setProperty("spa.frontendModules.@openmrs/" + moduleName, version);

properties.setProperty("spa.frontendModules." + moduleName, version);

ibacher · 2024-08-20T17:34:07Z

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

+     * Type of the dependency to add
+     */
+    @Parameter(property = "type")
+    private String type;


It would be good to document the acceptable values here so user's don't have to guess.

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

ibacher · 2024-08-20T17:36:23Z

sdk-commons/pom.xml

+        <dependency>
+            <groupId>com.google.code.gson</groupId>
+            <artifactId>gson</artifactId>
+            <version>2.10.1</version>
+            <scope>compile</scope>
+        </dependency>


Can we just use Jackson (already a dependency)?

ibacher · 2024-08-20T17:38:07Z

sdk-commons/src/main/java/org/openmrs/maven/plugins/utility/NPMHelper.java

+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class NPMHelper {


We've got this coming in as well. Which is, I think, a little more correct (i.e., it can support alternative registries, support for registries requiring authentication, etc.)

@ibacher should I wait for it to get merged?

Yes, I think so.

ibacher · 2024-08-28T19:54:17Z

Parallel to remove, should this be called add?

wikumChamith · 2024-08-29T06:38:44Z

@ibacher I updated the PR.

ibacher · 2024-09-08T11:44:47Z

@wikumChamith Would you be able to fix the conflict please? I've also merged the other NPM version helper, so maybe we can switch to using that?

ibacher

Thanks @wikumChamith. Basically looks good. A couple of nits.

ibacher · 2024-09-09T12:30:45Z

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

+    private final String SPA_OPTION = "SPA";
+    private final String OWA_OPTION = "OWA";
+    private final String WAR_OPTION = "WAR";
+    private final String CUSTOM_OPTION = "Custom";


What's the idea here?

ibacher · 2024-09-09T12:30:59Z

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

+
+

Could we remove the unnecessary blank lines?

ibacher · 2024-09-09T12:31:14Z

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

+
+    private final String DEPENDENCY_TYPE_PROMPT = "Enter the type of dependency you need to add";
+
+    private final String OMOD_OPTION = "OMOD";


Better for these to be declared static

ibacher · 2024-09-09T12:31:39Z

sdk-commons/src/main/java/org/openmrs/maven/plugins/utility/NpmVersionHelper.java


 	private static JsonNode getPackageMetadata(String versionRange, String packageName) throws IOException, InterruptedException {
 		if (packageName == null || packageName.isEmpty()) {
+


Do we need this line?

wikumChamith · 2024-09-09T13:13:23Z

@ibacher I updated the PR.

ibacher · 2024-09-10T06:55:12Z

sdk-commons/src/main/java/org/openmrs/maven/plugins/utility/NpmVersionHelper.java

+
+			Process process = processBuilder.start();
+
+			BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()));


Wrap this in a try-with-resources so the reader always gets closed.

Suggested change

BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()));

try(BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()))) {

Also, instead of using the BufferedReader to read by line, please use the code from getPackageMetadata(); it's a little more efficient than a line reader for a case like this where we don't care about line-by-line format.

wikumChamith requested a review from ibacher August 20, 2024 01:48

ibacher requested changes Aug 20, 2024

View reviewed changes

wikumChamith force-pushed the O3-3433 branch 3 times, most recently from 4cd9ace to 2adacd5 Compare August 23, 2024 10:21

wikumChamith force-pushed the O3-3433 branch 2 times, most recently from f0f827e to da3ef7f Compare August 29, 2024 06:38

wikumChamith requested a review from ibacher August 29, 2024 06:38

wikumChamith force-pushed the O3-3433 branch from da3ef7f to ef44708 Compare September 9, 2024 11:57

ibacher reviewed Sep 9, 2024

View reviewed changes

wikumChamith force-pushed the O3-3433 branch from ef44708 to 796980f Compare September 9, 2024 12:37

wikumChamith requested a review from ibacher September 10, 2024 06:25

ibacher reviewed Sep 10, 2024

View reviewed changes

wikumChamith force-pushed the O3-3433 branch 2 times, most recently from c6f0fca to aa24d7e Compare September 10, 2024 08:11

ibacher approved these changes Sep 10, 2024

View reviewed changes

O3-3433: SDK Command: Add dependencies to distro.properties

afe5764

wikumChamith force-pushed the O3-3433 branch from aa24d7e to afe5764 Compare September 10, 2024 08:39

ibacher approved these changes Sep 10, 2024

View reviewed changes

ibacher merged commit b09b2ca into openmrs:master Sep 10, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

O3-3433: SDK Command: Add dependencies to distro.properties #283

O3-3433: SDK Command: Add dependencies to distro.properties #283

wikumChamith commented Aug 19, 2024

ibacher left a comment

ibacher Aug 20, 2024

ibacher Aug 20, 2024

ibacher Aug 20, 2024

ibacher Aug 20, 2024

ibacher Aug 20, 2024

ibacher Aug 20, 2024

wikumChamith Aug 23, 2024

ibacher Aug 23, 2024

ibacher commented Aug 28, 2024

wikumChamith commented Aug 29, 2024

ibacher commented Sep 8, 2024

ibacher left a comment

ibacher Sep 9, 2024

ibacher Sep 9, 2024

ibacher Sep 9, 2024

ibacher Sep 9, 2024

wikumChamith commented Sep 9, 2024

ibacher Sep 10, 2024

	properties.setProperty("spa.frontendModules.@openmrs/" + moduleName, version);
	properties.setProperty("spa.frontendModules." + moduleName, version);


		private final String DEPENDENCY_TYPE_PROMPT = "Enter the type of dependency you need to add";

		private final String OMOD_OPTION = "OMOD";


		private static JsonNode getPackageMetadata(String versionRange, String packageName) throws IOException, InterruptedException {
		if (packageName == null \|\| packageName.isEmpty()) {


		Process process = processBuilder.start();

		BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()));

	BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()));
	try(BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()))) {

O3-3433: SDK Command: Add dependencies to distro.properties #283

O3-3433: SDK Command: Add dependencies to distro.properties #283

Conversation

wikumChamith commented Aug 19, 2024

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

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

ibacher commented Aug 28, 2024

wikumChamith commented Aug 29, 2024

ibacher commented Sep 8, 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

wikumChamith commented Sep 9, 2024

Choose a reason for hiding this comment