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

feat: add quince compatibility for distro #57

Merged
merged 9 commits into from
Mar 28, 2024

Conversation

Asespinel
Copy link
Contributor

@Asespinel Asespinel commented Mar 15, 2024

This PR adds quince compatibility for the distro plugin.

Major changes

  1. Removed the "openedx-dev-dockerfile-post-python-requirements" patch since the private packages won't be installed in the extra_deps folder anymore.
  2. The enable-private packages command will run the tutor mounts add command for each private package and will write the right hook inside the plugin.py file so the private package will be built inisde the Docker file
  3. Removed the exclusion rule towards the tests packages inside the setup.py file since the packages inside the tests modules weren't being imported correctly.
  4. Changed minimum tutor version to 17.0.3 to include a fix to properly install private packages described here: fix: ensure mounted installable packages are installed as expected overhangio/tutor#1016

How to test

  1. Install distro using this branch
  2. enable distro using tutor plugins enable distro
  3. Make sure your config file has the correct syntax to add themes and packages (include private packages) it can look something like this:
DISTRO_EOX_MANAGE_DPKG:
  domain: github.com
  index: git
  name: eox-manage
  path: eduNEXT
  private: true
  protocol: ssh
  repo: eox-manage
  variables:
    development: {}
    production: {}
  version: v5.0.0
DISTRO_EOX_SUPPORT_DPKG:
  domain: github.com
  index: git
  name: eox-support
  path: eduNEXT
  private: true
  protocol: ssh
  repo: eox-support
  variables:
    development: {}
    production: {}
  version: v7.0.0
DISTRO_EOX_THEMING_DPKG:
  domain: github.com
  index: git
  name: eox-theming
  path: eduNEXT
  private: false
  protocol: https
  repo: eox-theming
  variables:
    development:
      EOX_THEMING_CONFIG_SOURCES:
      - from_eox_tenant_microsite_v2
      - from_django_settings
    production:
      EOX_THEMING_CONFIG_SOURCES:
      - from_eox_tenant_microsite_v2
      - from_django_settings
  version: v7.0.0
DISTRO_EXTRA_MIDDLEWARES:
- eox_core.middleware.PathRedirectionMiddleware
- eox_core.middleware.RedirectionsMiddleware
DISTRO_THEMES:
- domain: github.com
  name: ednx-saas-themes
  path: eduNEXT
  protocol: ssh
  repo: ednx-saas-themes
  version: edunext/quince.master
DISTRO_THEMES_NAME:
- bragi
- css-runtime
DISTRO_THEMES_ROOT: /openedx/themes
DISTRO_THEME_DIRS:
- /openedx/themes/ednx-saas-themes/edx-platform
- /openedx/themes/ednx-saas-themes/edx-platform/bragi-generator
- /openedx/themes/ednx-saas-themes/edx-platform/bragi-children

OPENEDX_EXTRA_PIP_REQUIREMENTS:
- eox-tenant==v10.0.0
- eox-core==v10.0.0
- eox-hooks==v6.0.0
- eox-audit-model==v4.0.0
- eox-tagging==v7.0.0

  1. Test the following commands :
tutor distro syntax-validator
tutor distro repository-validator
tutor distro enable-themes
tutor distro enable-private-packages
  1. Run tutor build images openedx and check if there was no error
  2. Launch your environment and inside your container run pip list and verify that all the packages are installed

@MaferMazu
Copy link
Contributor

This looked good, but if we install distro with pip install git+https://github.com/eduNEXT/tutor-contrib-edunext-distro.git@{version or branch} the location of the plugin.py changes.

Another feedback with less priority is that we may want to remove the step of adding the private package in the private.txt file and avoid the related sections of the tests. If we can remove that in this PR, it will be amazing, but if not, we don't need to block this PR for that.

@github-actions github-actions bot added size/m and removed size/s labels Mar 20, 2024
@Asespinel
Copy link
Contributor Author

@MaferMazu I refactored the code to fix that problem, please check it out again and let me know if anything else arises

@MaferMazu
Copy link
Contributor

This looks good so far.
The code looks good, and the commands work well. I'm double-checking the image from the private packages; I can bring you a review when the test is finished.

Can you add a warning or a message to the PR description that explains that we need tutor v17.0.3 to support enable-private-packages? And remember when we will merge this to put a breaking change that mentions that we needed to change the enable-private-packages to work with quince.

Copy link
Contributor

@MaferMazu MaferMazu left a comment

Choose a reason for hiding this comment

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

Looks good to me!
With these changes, we can install private packages properly and provide support for the quince release. Thanks for this @Asespinel 🙌

Note: I had problems with the themes, but it was because the example said palma.master for the saas-themes branch, and it's incorrect for this version. Can we change that and add info about why v17.0.3? This is the only feedback I have. Thanks again for this support, Andrés.

@dcoa dcoa changed the title added quince compatibility for distro feat: add quince compatibility for distro Mar 27, 2024
@Asespinel
Copy link
Contributor Author

Looks good to me! With these changes, we can install private packages properly and provide support for the quince release. Thanks for this @Asespinel 🙌

Note: I had problems with the themes, but it was because the example said palma.master for the saas-themes branch, and it's incorrect for this version. Can we change that and add info about why v17.0.3? This is the only feedback I have. Thanks again for this support, Andrés.

Sure thing, I'll add those changes to my description thanks for the advice.

@Asespinel Asespinel force-pushed the ase/added-quince-compatibility branch from 1a1cf50 to 79dbe16 Compare March 27, 2024 18:03
@Asespinel Asespinel force-pushed the ase/added-quince-compatibility branch from 79dbe16 to 19ea4a5 Compare March 28, 2024 03:21
@Asespinel Asespinel merged commit 4e78afd into master Mar 28, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants