Adding new MFEs causes `dev start` to fail with image pull error

kdmccormick commented 1 year ago

The README describes how to add extra MFEs to the plugin. I've used this method successfully in the past.

I'm trying to run the tutor-contrib-library-authoring-mfe plugin, which uses that same method in order to add frontend-app-library-authoring. I ran the following:

pip install -e 'git+https://github.com/openedx/frontend-app-library-authoring.git#egg=tutor-contrib-library-authoring-mfe&subdirectory=tutor-contrib-library-authoring-mfe'
tutor plugins enable library_authoring_mfe
tutor config save
tutor dev init -l library_authoring_mfe

This all worked fine. However when I try to start the MFE:

tutor dev start library-authoring

I get:

Pulling library-authoring (docker.io/overhangio/openedx-library-authoring-dev:15.0.5)...
ERROR: pull access denied for overhangio/openedx-library-authoring-dev, repository does not exist or may require 'docker login': denied: requested access to the resource is denied
Error: Command failed with status 1: docker-compose -f /home/kyle/openedx/tutor-root/env/local/docker-compose.yml -f /home/kyle/openedx/tutor-root/env/dev/docker-compose.yml -f /home/kyle/openedx/tutor-root/env/dev/docker-compose.tmp.yml --project-name tutor_nightly_dev up --remove-orphans --build library-authoring

It looks like Tutor is trying to pull the non-existent docker.io/overhangio/openedx-library-authoring-dev image. I think it should try to instead build that image, since we wouldn't expect it to exist in the overhangio DockerHub repo.

Looking at $(tutor config printroot)/env/dev/docker-compose.yml, I see:

  library-authoring:
      image: "docker.io/overhangio/openedx-library-authoring-dev:15.0.5"
      ports:
          - "3001:3001"
      stdin_open: true
      tty: true
      volumes:
          - ../plugins/mfe/apps/mfe/webpack.dev-tutor.config.js:/openedx/app/webpack.dev-tutor.config.js:ro
      restart: unless-stopped
      depends_on:
          - lms

I suspect this is related the the MFE runtime config change, because up until that change, all MFE images were build locally; none of them were pulled down from DockerHub.

tutor version: 15.2.0-nightly tutor-mfe version: 15.0.5 nightly

kdmccormick commented 1 year ago

@arbrandes or @ghassanmas , would either of you have time to take a look?

regisb commented 1 year ago

Kyle, would it be acceptable if we just amended the docs to say that developers must build the dev image themselves with tutor images build library-authoring-dev?

ghassanmas commented 1 year ago

other way to resolve would be in this patch https://github.com/overhangio/tutor-mfe/blob/master/tutormfe/patches/local-docker-compose-dev-services instead of iterating on all MFE_APP suffix we only iterate on the one that we officially support i.e. have an image built for them. OR we could add if clause at the image part and only write if it in the official repos. I am just putting another opinion on the table I haven't tested consequence of this approahce if exits

kdmccormick commented 1 year ago

@regisb tutor images build library-authoring-dev does not work because that image is not added to IMAGES_BUILD; it's only defined in dev/docker-compose.yml, just like the openedx-dev. I tried building it using dev dc, but that docker-compose doesn't like that either:

$ tutor dev dc build library-authoring
docker-compose -f /home/kyle/openedx/tutor-root/env/local/docker-compose.yml -f /home/kyle/openedx/tutor-root/env/dev/docker-compose.yml -f /home/kyle/openedx/tutor-root/env/dev/docker-compose.tmp.yml --project-name tutor_nightly_dev build library-authoring
library-authoring uses an image, skipping

arbrandes commented 1 year ago

For the record, I like @ghassanmas's idea. The *_MFE_APP iteration coupled with the assumption that the image exists results in surprising behavior, which I think we should avoid. We should instead only iterate on a list composed of specific MFEs.

Relatedly, @kdmccormick, FYI @brian-smith-tcril is working on improvements to that plugin, which you may want to look at.

regisb commented 1 year ago

instead of iterating on all MFE_APP suffix we only iterate on the one that we officially support

This would mean that we no longer support adding MFE apps via the "*_MFE_APP" config, right? I agree that this extension mechanism is a little clunky. It could certainly be improved thanks to the plugin v1 API, which did not exist at the time we created tutor-mfe.

With @ghassanmas' suggestion we would no longer have issues starting the custom MFEs in development mode, but we would also lose the possibility of running custom MFEs in a development environment. So we need to find a better solution.

tutor images build library-authoring-dev does not work because that image is not added to IMAGES_BUILD

Right. But if it did work, would it be an acceptable solution @kdmccormick? Or would we need something more transparent, like auto-build on start?

kdmccormick commented 1 year ago

I think there are two issues here:

Since the runtime config change, MFE dev images are always pulled, never built. This works for builtin MFEs, but not for custom MFEs.
The *_MFE_APP app system is a little janky. Funnily enough, it reminds me of a Filter: it's a global list of things you can iterate over and that plugins can extend. Given that, it seems like we should modernize it by turning it into a proper filter.

I think we can solve these both, but I want to consider the solutions separately:

Smaller proposal: just fix (1)

We add a new field to the *_MFE_APP dictionaries: "dev_image_url". When that is set to a remote URL, such as {{ DOCKER_REGISTRY }}overhangio/openedx/authn-dev, then the dev image will be pulled. When it let unset, the image will be built locally. That way, custom MFEs can be added, and if their "dev_image_url" is not set, then the dev image will be built locally just like before.

Implementing this would mean adding some additional conditional logic to the dev/docker-compose.yml template.

Bigger proposal: fix (1) and (2)

We create a new Filter, call it MFE_APPS, which yields a of dictionary like:

{
    "authn": {
        # These are the same values that are in *_MFE_APP today.
        "repository": "https://github.com/openedx/frontend-app-authn",
        "port": 1999,
        "version": "open-release/nutmeg.2",  # Optional. Defaults to {{ MFE_COMMON_VERSION }}.

        # This is the new dev_image_url proposal.
        # Optional. Defaults to None, in which case it's always built instead of pulled.
        "dev_image_url": "{{ DOCKER_REGISTRY }}overhangio/openedx/authn-dev", 
    },
    "account": {
        "repository": ...,
        "port": ...,
    },
    "profile": {
        ...,
    },
    ...,
}

User plugins could add or modify MFEs like such:

# (aside: This would be our first instance of having a plugin-defined hook.
#         Here, I suggest a convention: plugins can define custom Actions
#         and Filters catalog classes under <pluginpackage>/hooks.py)
#         an define their own Actions and Filters catalogs under <package>/hooks.py)
from tutormfe.hooks import Filters as MfeFilters

MfeFilters.MFE_APPS.add()
def _add_cool_new_mfe(mfe_apps: dict) -> dict:
    mfe_apps["coolnew"] = {
        "repository": "https://github.com/arbrandes/frontend-app-cool-new-feature",
        "port": 2023,
    }

MfeFilters.MFE_APPS.add()
def _use_custom_profile_version(mfe_apps: dict) -> dict:
    mfe_apps["profile"]["repository"] = "https://github.com/arbrandes/frontend-app-profile"
    mfe_apps["profile"]["version"] = "arbrandes/profile-rework"

Inside tutor-mfe, the MFE_APPS filter would be used where the *_MFE_APPS settings are used right now. We would deprecate *_MFE_APPS, although we could add a backwards-compatibility shim for the next release or two.

One consideration with this proposal is that users could no longer add or modify MFEs just with tutor config; they'd need to create a plugin for any change. Given that we now require a plugin just to change MFE app settings, though, I think this change is in line with our current direction.

kdmccormick commented 1 year ago

tutor images build library-authoring-dev does not work because that image is not added to IMAGES_BUILD

Right. But if it did work, would it be an acceptable solution @kdmccormick? Or would we need something more transparent, like auto-build on start?

@regisb , I think it would be acceptable if forgetting to build led you to a helpful error message. Something like "image does not exist" seems OK to me, but "pull access denied" does not. Auto-build on start would be even better.

brian-smith-tcril commented 1 year ago

@kdmccormick re: proposed solution 2 above: In this scenario, would custom MFEs (or tutor plugins used to install the custom MFEs) include the code to _add_cool_new_mfe?

kdmccormick commented 1 year ago

@brian-smith-tcril Yes.

arbrandes commented 1 year ago

I vote for @kdmccormick's "Bigger Proposal". It reads very much in line with the rest of the landscape.

regisb commented 1 year ago

Yes, Kyle's "bigger proposal©" is great.

A couple of notes:

Not a fan of "dev_image_url": this introduces yet another Docker image setting. Instead, we can simply check for the presence of "mymfe-dev" in Filters.IMAGES_PULL. If not present, then build at runtime. Actually, now that I think of it, we can implement this change right now to auto-build dev images without introducing any breaking change.
I don't even think we need a compatibility shim: just make it a breaking change in Palm/nightly.
I'm not 100% sure about this one yet, but I feel like MFE_APPS should be a list, not a dict, because order might be important. Also, lists play better with Filter.add_item.
Licensing: tutor-mfe is AGPL, but we (at least: I) don't want to force the AGPL on other plugin developers. The Tutor hooks API is Apache-licensed. I suggest we do the same for the tutormfe.hooks API.

kdmccormick commented 1 year ago

Not a fan of "dev_image_url": this introduces yet another Docker image setting. Instead, we can simply check for the presence of "mymfe-dev" in Filters.IMAGES_PULL. If not present, then build at runtime. Actually, now that I think of it, we can implement this change right now to auto-build dev images without introducing any breaking change.

💯

I'm not 100% sure about this one yet, but I feel like MFE_APPS should be a list, not a dict, because order might be important. Also, lists play better with Filter.add_item.

@regisb Yeah, I went back and forth on this one. I chose the dict because it lends itself well to partially or fully overriding parameters for an MFE, like:

MfeFilters.MFE_APPS.add()
def _use_custom_profile_version(mfe_apps: dict) -> dict:
    mfe_apps["profile"]["repository"] = "https://github.com/arbrandes/frontend-app-profile"
    mfe_apps["profile"]["version"] = "arbrandes/profile-rework"
    return mfe_apps

How would you see that working with a list? Perhaps: if multiple MFE_APPS entries exist with the same name, use the last one?

regisb commented 1 year ago

if multiple MFE_APPS entries exist with the same name, use the last one?

~I don't think I would treat that as a special case. docker-compose will crash on discovering multiple services with the same name, and I'm fine with that. Else, just raise a TutorError.~

EDIT: no, this is not a good idea. See below.

kdmccormick commented 1 year ago

@regisb If we go with that, then changing the repository or branch for an MFE would require something like this:

MfeFilters.MFE_APPS.add()
def _use_custom_profile_version(mfe_apps: list[dict]) -> list[dict]:
    for mfe_app in mfe_apps:
        if mfe_app["name"] == "profile":
            mfe_app["repository"] = "https://github.com/arbrandes/frontend-app-profile"
            mfe_app["version"] = "arbrandes/profile-rework"
    return mfe_apps

Do you all like that? Personally, I feel like this is too much code for such a common task. Perhaps I'm overestimating how often users will want to override an MFE's repo & branch.

regisb commented 1 year ago

Do you all like that? Personally, I feel like this is too much code for such a common task.

I agree, it's too much. You're right, I think we should go with a dict and your initial proposal.

Hina-softwareEngineer commented 1 year ago

Hello everyone, I was just going through the same issue that is I was unable to start tutor dev start when added my custom_mfe. Got this error.

Error response from daemon: pull access denied for overhangio/openedx-custom_mfe-dev, repository does not exist or may require 'docker login': denied: requested access to the resource is denied

Also, I was unable to build the custom-mfe image for dev environment using tutor images build custom_mfe-dev. [getting no image of this name exist]

The reason I found is that in tutor-mfe/plugin.py there is a tuple named ALL_MFES which is used to build images for dev environment. I have added my custom_mfe in that tuple and then tutor images build custom_mfe-dev worked perfectly.

I propose the below solution to that.

Add custom_mfe name as a list in tutor config variable like [example: tutor config save --set CUSTOM_MFES="['mfe1', 'mfe2']"]. and then pick its value in tutor-mfe and add to the ALL_MFES variable.

Looking forward for your thoughts.

arbrandes commented 1 year ago

@Hina-softwareEngineer, I think we're going to go with @kdmccormick initial proposal. It achieves the same results as yours, but with plugins instead of configuration variables. (Which, at this point, we prefer.)

The only thing is that currently nobody is working on an implementation.

regisb commented 1 year ago

Working on this for Palm.

regisb commented 1 year ago

Hey @kdmccormick, @arbrandes, I'm really pleased with how this fix turned out. Please have a look at the upcoming changes for Palm, and in particular the README: https://github.com/overhangio/tutor-mfe/commit/9e9f94df7c58562e8480c36e85e5ca3831052c7f#diff-7b3ed02bc73dc06b7db906cf97aa91dec2b2eb21f2d92bc5caa761df5bbc168f

The workflow is short and sweet, I think:

tutor config save --append MOUNTS=/path/to/frontend-app-profile
tutor dev launch # tutor local launch works as well

Eager to discuss this with you. I'll present this during the next DevEx meetup on Monday.

arbrandes commented 1 year ago

I tested the changes, and I approve whole-heartedly!

overhangio / tutor-mfe

Adding new MFEs causes `dev start` to fail with image pull error #111

Smaller proposal: just fix (1)

Bigger proposal: fix (1) and (2)