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.

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-306: Integrated Micro Frontend Tooling Setup in OpenMRS SDK - Reactoring #224

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

wikumChamith
Copy link
Contributor

This is to address issues with a41bab6

Jira ticket: https://issues.openmrs.org/browse/SDK-306

@dkayiwa
Copy link
Member

dkayiwa commented Jun 23, 2023

@wikumChamith can you look at the build failures?

@wikumChamith
Copy link
Contributor Author

wikumChamith commented Jun 23, 2023

@wikumChamith can you look at the build failures?

@dkayiwa I think they got fixed with this. Is there a way to re-run checks without pushing again??

@wikumChamith
Copy link
Contributor Author

@wikumChamith can you look at the build failures?

@dkayiwa I think they got fixed with this. Is there a way to re-run checks without pushing again??

@dkayiwa I merged master into this.

@@ -779,7 +779,7 @@ private Map<String, String> getDistroVersionsOptionsMap(Set<String> versions, Ve
private Map<String, String> getO3VersionsOptionsMap(VersionsHelper versionsHelper,
String optionTemplate, String artifactTemplate) {
Map<String, String> optionsMap = new LinkedHashMap<>();
Artifact artifact = new Artifact("referenceapplication-distro", "3.0.0-SNAPSHOT", "org.openmrs.distro", "zip");
Artifact artifact = new Artifact("referenceapplication-distro", null, "org.openmrs.distro", "zip");
Copy link
Member

Choose a reason for hiding this comment

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

I remember suggesting passing NULL for the version parameter and you said it was not allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dkayiwa apologies for the confusion. When evaluating with the debugger I encountered an exception when using null. So I decided not to use null. Upon further investigation, I realized that even though an exception gets thrown, the code was still able to fetch the versions list.

Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid the exception being thrown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dkayiwa I am sorry, I have made a mistake. I accidentally ran
mvn org.openmrs.maven.plugins:openmrs-sdk-maven-plugin:4.6.0-SNAPSHOT:setup instead of mvn org.openmrs.maven.plugins:openmrs-sdk-maven-plugin:4.7.0-SNAPSHOT:setup when testing new code changes.
We can't still use null. If we pass null we get the following error.
[ERROR] Failed to execute goal org.openmrs.maven.plugins:openmrs-sdk-maven-plugin:4.7.0-SNAPSHOT:setup (default-cli) on project openmrs-sdk: Failed to setup server: For artifact {org.openmrs.distro:referenceapplication-distro:null:}: The version cannot be empty.
I am changing the argument to "version".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dkayiwa I am really sorry, I have made a mistake. I accidentally ran
mvn org.openmrs.maven.plugins:openmrs-sdk-maven-plugin:4.6.0-SNAPSHOT:setup instead of mvn org.openmrs.maven.plugins:openmrs-sdk-maven-plugin:4.7.0-SNAPSHOT:setup when testing new code changes.

We can't still use null. If we pass null we get the following error.

[ERROR] Failed to execute goal org.openmrs.maven.plugins:openmrs-sdk-maven-plugin:4.7.0-SNAPSHOT:setup (default-cli) on project openmrs-sdk: Failed to setup server: For artifact {org.openmrs.distro:referenceapplication-distro:null:}: The version cannot be empty.

I am changing the parameter to "version".

@dkayiwa
Copy link
Member

dkayiwa commented Jun 26, 2023

@wikumChamith can we also let someone choose the version of the openmrs assemble and build tools to use? The default can remain latest as it currently is. But allow someone to override it.
I guess it will help solve problems like this: https://talk.openmrs.org/t/cannot-build-the-o3-reference-ui/39959 where he is using the default latest which is unstable because of the current significant frontend rewrite and related changes. So it would be great to have an option of using stable previous versions. You can create a separate pull request for this.

npm exec -- openmrs@latest --version

@wikumChamith
Copy link
Contributor Author

@wikumChamith can we also let someone choose the version of the openmrs assemble and build tools to use? The default can remain latest as it currently is. But allow someone to override it. I guess it will help solve problems like this: https://talk.openmrs.org/t/cannot-build-the-o3-reference-ui/39959 where he is using the default latest which is unstable because of the current significant frontend rewrite and related changes. So it would be great to have an option of using stable previous versions. You can create a separate pull request for this.

npm exec -- openmrs@latest --version

@dkayiwa should these get included in the distro.properties?

@dkayiwa
Copy link
Member

dkayiwa commented Jun 26, 2023

should these get included in the distro.properties?

Can i also pass it in as a command line argument?

@wikumChamith
Copy link
Contributor Author

should these get included in the distro.properties?

Can i also pass it in as a command line argument?

Then we should add new maven parameters for them. @dkayiwa could you create a new ticket for this with more details?? We can discuss this there.

@dkayiwa
Copy link
Member

dkayiwa commented Jun 26, 2023

Yes you can create a ticket.

@wikumChamith
Copy link
Contributor Author

Yes you can create a ticket.

@dkayiwa I created a ticket: https://issues.openmrs.org/browse/SDK-312

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants