Add support to passing a custom spa-build-config.json file#316
Add support to passing a custom spa-build-config.json file#316wikumChamith wants to merge 1 commit intoopenmrs:masterfrom
Conversation
wikumChamith
force-pushed
the
spaConfig
branch
from
9a36741 to
5c5fb1f
Compare
|
|
||
| @Parameter(property = "spaConfigFile") | ||
| private String spaConfigFile; | ||
|
|
There was a problem hiding this comment.
I don't really understand how this would be used in the build-distro goal. A distribution is intended to be something that is fully represented in an openmrs-distro.properties file. So any configuration needs to be able to be represented in that file - either as a spa.xxx property, or as coordinates to an artifact of a known type. This is why/how we have used content packages as this known artifact/type.
In fact, I would rather see us remove most of the properties that are currently in this job, rather than add new ones, as I think most are probably ill-advised, and would have been better represented as configuration options inside of the openmrs-distro.properties file itself. @ibacher - thoughts?
| artifact = wizard.promptForRefApp3xArtifact(versionsHelper); | ||
| } | ||
| distribution = builder.buildFromArtifact(artifact); | ||
| distribution = builder.buildFromArtifact(artifact, spaConfigFile); |
There was a problem hiding this comment.
Having to bolt on this extra property here is an indication that this isn't the way other things work in building the distro, and I'd like to avoid adding these special cases in.
| public Boolean overrideReuseNodeCache; | ||
|
|
||
| @Parameter(property = "spaConfigFile") | ||
| public String spaConfigFile; |
There was a problem hiding this comment.
I think we should revisit the entire set of properties around how O3 is included in the SDK, before we add new properties in. For example - why do we have appShellVersion and ignorePeerDependencies, and overrideReuseNodeCache here, and should they actually not be here. And how does the spaConfigFile relate to this or not. @ibacher . My gut is that all of these should be removed in favor of something more aligned with the rest of the SDK and distributions.
| Artifact artifact = wizard.promptForRefApp3xArtifact(versionsHelper); | ||
| Distribution distribution = builder.buildFromArtifact(artifact); | ||
| return distribution.getEffectiveProperties(); | ||
| Distribution distribution = builder.buildFromArtifact(artifact, spaConfigFile); |
| throw new RuntimeException(e); | ||
| } | ||
| } | ||
| else if (new Version(distroVersion).higher(new Version("3.0.0-beta.16"))) { |
There was a problem hiding this comment.
The fate of this will depend on the other decisions we make in the questions I raise above, but we should review the hacks that already exist to support refapp 3.x.x here as we look at how best to support these use cases moving forward.
2 participants
Description of what I changed
Issue I worked on
see https://openmrs.atlassian.net/browse/SDK-367
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 --amendI 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 --amendI ran
mvn clean installright before creating this pull request andadded 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