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: introduces MFE_DOCKER_IMAGE_DEV_PREFIX #216

Merged

Conversation

rediris
Copy link
Contributor

@rediris rediris commented Jun 20, 2024

Introduces a MFE_DOCKER_IMAGE_DEV_PREFIX setting to customize dev MFE image docker repos. See discussion below for the benefits.

@rediris
Copy link
Contributor Author

rediris commented Jun 20, 2024

@regisb thoughts?

@rediris rediris changed the title [Feature]: enhanced docker image tag feat: enhanced docker image tag Jun 20, 2024
@regisb
Copy link
Contributor

regisb commented Jun 21, 2024

I'm not sure I understand. What problem exactly is this PR solving? Please describe your use case precisely, and how this change improves your workflow.

@rediris
Copy link
Contributor Author

rediris commented Jun 21, 2024

Yes, this probably deserves a bit more explanation than what I provided above.

Scenario 1: when customizing existing MFEs
Using a custom docker repo for MFE development images is currently impossible because the overhangio images are hard-coded into the plugin. While it is possible to override the tutor local mode production MFE image by using MFE_DOCKER_IMAGE in config.yml, this does not apply to the individual MFE images when running tutor in dev mode. This creates a bit of a development bottleneck, as the dev is forced to manually build/rebuild the development images instead of expediently pulling them from the custom docker repo. Additionally, the current process seems to necessitate using --no-cache and/or --no-registry-cache flags to prevent automatically pulling the default overhangio images.

Scenario 2: when creating new MFEs
The hard-coded overhangio image references create a similar problem when creating new MFEs. Let's say I have a custom MFE called frontend-app-customizeme - the resulting docker image path will look like
docker.io/overhangio/openedx-customizeme-dev:17.0.1 (based on what I'm seeing here: https://github.com/overhangio/tutor-mfe/blob/master/tutormfe/plugin.py#L154-L155).

Let me know if any of that makes sense...

@regisb
Copy link
Contributor

regisb commented Jun 21, 2024

OK I understand. For once, the "right" fix for this is to introduce a new setting: MFE_DOCKER_IMAGE_DEV_PREFIX="{{ DOCKER_REGISTRY }}overhangio/openedx-". Can you please change your PR to follow this approach? Please remember to include a changelog entry: https://docs.tutor.overhang.io/tutor.html#contributing

@rediris
Copy link
Contributor Author

rediris commented Jun 21, 2024

OK I understand. For once, the "right" fix for this is to introduce a new setting: MFE_DOCKER_IMAGE_DEV_PREFIX="{{ DOCKER_REGISTRY }}overhangio/openedx-". Can you please change your PR to follow this approach? Please remember to include a changelog entry: https://docs.tutor.overhang.io/tutor.html#contributing

Will do, thank you very much!

@rediris
Copy link
Contributor Author

rediris commented Jun 21, 2024

@regisb all set, let me know what you think!

@rediris
Copy link
Contributor Author

rediris commented Jun 21, 2024

Oops, forgot the changelog...

@rediris rediris changed the title feat: enhanced docker image tag feat: introduces MFE_DOCKER_IMAGE_DEV_PREFIX Jun 21, 2024
@rediris rediris changed the title feat: introduces MFE_DOCKER_IMAGE_DEV_PREFIX feat: introduces MFE_DOCKER_IMAGE_DEV_PREFIX Jun 21, 2024
Resolves `Error: Image 'course-about-dev' could not be found`
rediris added a commit to gymnasium/gym-custom-tutor-config that referenced this pull request Jun 21, 2024
Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

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

lgtm. @arbrandes what do you think?

@hinakhadim
Copy link
Contributor

I have tested it. LGTM!

@DawoudSheraz DawoudSheraz merged commit eab8bba into overhangio:master Jul 1, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants