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

Adds an Ozone Implementation archetype #40

Merged
merged 15 commits into from
Feb 16, 2024

Conversation

ibacher
Copy link
Contributor

@ibacher ibacher commented Feb 6, 2024

This adds an Maven archetype that creates a skeleton Ozone Implementation project

@ibacher
Copy link
Contributor Author

ibacher commented Feb 6, 2024

I don't know why the validate step is failing. @corneliouzbett any ideas?

@corneliouzbett
Copy link
Contributor

corneliouzbett commented Feb 7, 2024

I don't know why the validate step is failing. @corneliouzbett any ideas?

Secrets are not passed to the runner when a workflow is triggered from a forked repository. So both username and password will be empty in the settings.xml. This is a security measure to prevent unauthorized access to sensitive data.

I believe, we don't need secrets for the validation step for this repo but we need for private repos.

I've added a PR here to conditionally, skip set the setting.xml step using the input variable use-secrets which is false by default.

@rbuisson
Copy link
Contributor

rbuisson commented Feb 7, 2024

@corneliouzbett @ibacher , the PR to fix the workflow is merged.

Copy link
Contributor

@rbuisson rbuisson left a comment

Choose a reason for hiding this comment

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

Thanks @ibacher!

  • Would it be possible to have the interactive mode to default the version to 1.0.0-SNAPSHOT instead of 1.0-SNAPSHOT?
    Define value for property 'version' 1.0-SNAPSHOT:

  • The default location for config files for OpenMRS for instance should be configs/openmrs/(frontend_config|initializer_config)/.

  • It sounds like this is a distro-only archetype. Shouldn't it be a complete Ozone archetype which sets up the distro sub module but will also bring the run/ resources upon build?

  • This adds some overhead to maintain duplicated version of the scripts files. Is there a way to reuse the existing actual Ozone resources as archetype resources? Or the opposite, Ozone to directly use the archetype resources?

  • The location of the Maven Wrapper is the root directory. In Ozone, we placed it in scripts/ . Is there an easy way to install it to the same location? My concern is that the Ozone documentation will not work on a project created from this archetype if the Maven wrapper is not at the same location.

@ibacher
Copy link
Contributor Author

ibacher commented Feb 7, 2024

Shouldn't it be a complete Ozone archetype which sets up the distro sub module but will also bring the run/ resources upon build?

The idea of the archetype is to make it easier to setup distro-specific projects, since that's what there will be multiple versions of, so this is designed to produce something like ozone-distro-cambodia or ozone-kenyahmis. Do we have a use-case for multiple things with a distro submodule that pull in the run resources on build?

This adds some overhead to maintain duplicated version of the scripts files. Is there a way to reuse the existing actual Ozone resources as archetype resources? Or the opposite, Ozone to directly use the archetype resources?

We can probably abuse the fact that they're all submodules of a single project to do something.

This is modeled pretty concretely on what we're already doing with distributions (go-to-scripts-dir.sh needs to be filtered and then set-ozone-dir.sh relies on go-to-scripts-dir.sh being in the same folder), so it's not added maintenance over-and-above what already happens with ozone-cambodia or ozone-kenyahmis.

The location of the Maven Wrapper is the root directory.

Yeah, that's trivial to do, although I think putting the wrapper in a sub-directory is a little odd. The README for the generated projects uses instructions without having the Maven Wrapper in the scripts dir, but that's also fixable.

@ibacher ibacher force-pushed the add-archetype-refactor-parent branch from 740deb9 to c03a8fe Compare February 7, 2024 13:47
@ibacher
Copy link
Contributor Author

ibacher commented Feb 7, 2024

For the tests here to work, I need the Maven job to run the install step and not just the verify step. Is there a way to do that?

The problem is that the archetype includes a default integration test that actually needs things to be installed locally to work.

@corneliouzbett
Copy link
Contributor

corneliouzbett commented Feb 7, 2024

For the tests here to work, I need the Maven job to run the install step and not just the verify step. Is there a way to do that?

Yes, by modifying this to run install or take input variable maven-phase which runs verify by default.

Here is the PR to enable that.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@rbuisson
Copy link
Contributor

rbuisson commented Feb 7, 2024

The idea of the archetype is to make it easier to setup distro-specific projects, since that's what there will be multiple versions of, so this is designed to produce something like ozone-distro-cambodia or ozone-kenyahmis. Do we have a use-case for multiple things with a distro submodule that pull in the run resources on build?

@ibacher , so I just want to make sure that by default the "distribution" that will use this artefact will inherit/use/depend on the whole ozone project, not only the ozone-distro part, as we need the projects to be able to run the suite.

@rbuisson
Copy link
Contributor

rbuisson commented Feb 7, 2024

We can probably abuse the fact that they're all submodules of a single project to do something.

This is modeled pretty concretely on what we're already doing with distributions (go-to-scripts-dir.sh needs to be filtered and then set-ozone-dir.sh relies on go-to-scripts-dir.sh being in the same folder), so it's not added maintenance over-and-above what already happens with ozone-cambodia or ozone-kenyahmis.

Well, it does add a little bit of maintenance in the sense that it wasn't ours to do before. Now the file is hosted in the repo, that's upon us to maintain the 2 versions.
I'm ok with having this duplication right now. And see to improve it later. Anyway some of those scripts are already duplicated in Ozone Docker Compose too. A little piece of work there.

@ibacher
Copy link
Contributor Author

ibacher commented Feb 7, 2024

@rbuisson So the projects created by this archteyp use the ozone-distro-parent as a parent. ozone-distro-parent adds the full Ozone distribution as a dependency and the various default steps to work with it.


Re-build:
```bash
./scripts/mvnw clean package
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to assume the Maven Wrapper is in scripts/ though, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved it and updated the text here.

@rbuisson
Copy link
Contributor

rbuisson commented Feb 7, 2024

Yeah, that's trivial to do, although I think putting the wrapper in a sub-directory is a little odd.

I wanted to keep the root directory as clean as possible. But if that's an unusual thing to do and Maven Wrapper is 99% of the time located at the root folder, let's do have it there. We can modify Ozone itself accordingly in a follow up commit.

@ibacher ibacher changed the title Adds an Ozone Distribution archetype Adds an Ozone Implementation archetype Feb 8, 2024
README.md Outdated Show resolved Hide resolved
maven-archetype/pom.xml Outdated Show resolved Hide resolved
maven-archetype/pom.xml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
tasks:
- name: Run Ozone ${distributionName}
before: sudo apt-get update && sudo apt-get install -y gettext-base && sudo rm -rf /var/lib/apt/lists/*
init: mvnw clean package
Copy link
Contributor

Choose a reason for hiding this comment

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

Prob needs to be:

scripts/mvnw clean package

@rbuisson
Copy link
Contributor

For some reason the final package ends up missing the readme/impl-guide.md file.

myOzone/
├── README.md
├── config
│   └── openmrs
│       ├── frontend_config
│       └── initializer_config
├── pom.xml
└── scripts
    ├── go-to-scripts-dir.sh
    ├── mvnw
    ├── mvnw.cmd
    ├── set-ozone-dir.sh
    └── start-ozone.sh

@ibacher
Copy link
Contributor Author

ibacher commented Feb 12, 2024

Re: readme/impl-guide.md: My fault. The archetype only picks up on files described in the src/main/resources/META-INF/maven/archetype-metadata.xml descriptor. I mistakenly left that out.

@ibacher
Copy link
Contributor Author

ibacher commented Feb 12, 2024

Also, the scripts have been removed from this PR, but they're added back with #42.

Copy link
Contributor

Choose a reason for hiding this comment

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

This file does not seem to be included in the final package. Is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No... I guess it's this issue. Adding a work-around.

@rbuisson
Copy link
Contributor

@ibacher do you plan to send a PR on Ozone Docs too?

@ibacher
Copy link
Contributor Author

ibacher commented Feb 15, 2024

@rbuisson PR for docs is here.

@rbuisson rbuisson merged commit 51f7ba9 into ozone-his:main Feb 16, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants