Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Upgrade InvenioRDM to v12 and improve M1 Mac Compatibility #389

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tmorrell
Copy link

This pull request upgrades InvenioRDM to the current version v12. The setup is based on https://github.com/inveniosoftware/cookiecutter-invenio-rdm and a couple of tweaks to get it working integrated with pass. I was able to sort out the docker networking issues, so this is a fully working example.

It also adds the platform to the docker compose files, which is needed on M1 macs.

@rpoet-jh rpoet-jh self-requested a review December 11, 2024 20:50
@rpoet-jh
Copy link
Contributor

Thanks @tmorrell for taking the time to test the PASS/InvenioRDM integration and for opening this PR, we really do appreciate your time and feedback.

We've reviewed the PR, and we have a few questions/comments. We know you have already spent time with this effort, and we don't want to take any more of your time unless you would like to continue. Let us know if you like to put more time on the PR, or should we take it over and shepherd it into the codebase (we should be able to retain your commits).

Review questions/comments:

  • The platform change to the docker compose files would cause an issue for the case where we build say pass-core docker image locally on a mac/M1 and want to test it in pass-docker. With the platform attribute, docker will pull the linux/amd64 version and overwrite the just built image. I run pass-docker on a mac with M1 with the configuration in pass-docker on the main branch. I use Docker Desktop with Apple Virtualization framework and Rosetta enabled, and the containers start up successfully. It could be a Docker Desktop/Engine version difference if you have these virtualization options already enabled (I have the latest version Docker Desktop 4.36.0/Engine 27.3.1). The other thing with this issue is we do have an existing ticket to create multi-arch docker images which would end up solving this issue, so we can talk with the group to possibly reprioritize this issue.

  • Thanks again for upgrading InvenioRDM to version 12. A few comments here:

    • Since the app_data/vocabularies files are not configured to be loaded in vocabularies.yaml, we would prefer to leave them out, especially affiliations_ror.yaml given the size. The main objective for this test configuration is to test deposits. If someone needs custom vocabularies, they can always get these files from the InvenioRDM github repo.
    • We noticed your path in the .invenio file for _output_dir. This was set to ./ in the previous version to make it user independent.
    • We would remove the addition to the README since we have created start.sh and stop.sh scripts to make it easier for the user. This is also what is in our docs: https://docs.eclipse-pass.org/developer-documentation/pass-docker/invenio-rdm
  • PASS is within the Eclipse Foundation organization, so it requires that all commit authors have an ECA with eclipse, which it looks like you do have based on the PR status check https://api.eclipse.org/git/eca/status/gh/eclipse-pass/pass-docker/389. We were curious if you had to sign an ECA with Eclipse when you opened the PR, or if you have done it in the past at some point?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants