invert dependencies so that backends now depend on frontends (plus some cleanup) #783

kdmccormick · 2021-06-17T07:29:28Z

Description

The intent of this PR is to resolve an immediate developer experience issue: since the Learning MFE is now enabled by default, the command make dev.up.lms on its own is no longer sufficient to run the default Open edX courseware experience.

We hope that the fix for this issue will help us resolve a deeper, upcoming developer experience issue: soon ¹, several micro-frontends will need to be started alongside backend services in order to provide a working Open edX experience out-of-the-box.

Changes are best reviewed by-commit.

The first commit is some doc fixes related to devserver restarting.
The next commit is a superficial-yet-breaking interface change in order to make frontend app names consistent in Devstack. We are doing this clean-up opportunistically, since we'll need to announce workflow-breaking changes from this PR anyway.
The next commit is a non-user-facing style cleanup (some YAML alphabetization).
The next commit is a non-user-facing refactor (pulling XQueue into the main docker-compose.yml file).
The penultimate commit is a major interface change, along with an ADR. In short, backend services will be defined as depending on frontend services, such that make dev.up.<backend-service> automatically starts all required frontends service.
The final commit makes it so local NPM packages can be used with devstack microfrontends. There is an included ADR, and I am making frontend-build doc updates here.

I will communicate all interface changes via email and Slack.

Supporting information

TNL-8407

Further details are in the included ADRs (0004 and 0005).

Related PR to update frontend-build docs: openedx/frontend-build#193

Testing instructions

Suggested smoke test for service dependencies:

Ensure that make dev.up.lms brings up at least LMS, frontend-app-gradebook and frontend-app-learning. Ensure that those services are usable.
Ensure that make dev.attach.frontend-app-gradebook attaches to the Gradebook MFE container.
Ensure that make dev.up.xqueue brings up XQueue and MySQL 5.7.
Ensure that make dev.down brings down all services.

Suggested smoke test for local package mounting:

Start LMS and fa-learning with make lms-up.
Load the demo course, confirming that it loads in the Learning MFE using the default (bootstrap blue) Open edX theme.
Clone https://github.com/edx/brand-edx.org into the src/ directory within your devstack workspace.
Run npm install && npm build in that repo.
Follow these update docs, copying module.config.js into frontend-app-learning.
In module.config.js, set @edx/brand to the directory ../src/brand-edx.org. Comment out all other local modules.
Shell into frontend-app-learning (make frontend-app-learning-shell) and run npm install. Exit shell.
Restart frontend-app-learning.
Load the demo course again, confirming that it shows the edX.org elm-green theme.

When? Not sure; it's up to individual squads that own MFEs. ~~We do know that Engage squad has work in-flight to make the Course Home features run in the Learning MFE by default.~~ (update: they just did!) Also, several MFEs run as the default in Lilac, so it would make sense if those were made to run in Devstack by default as well. ↩

kdmccormick

Notes/questions for reviewers + some typos I will fix.

kdmccormick · 2021-06-17T13:55:36Z

docker-compose-host-nfs.yml

@@ -45,22 +45,22 @@ services:
  registrar-worker:
    volumes:
      - ${DEVSTACK_WORKSPACE}/registrar:/edx/app/registrar/registrar
-  gradebook:
+  frontend-app-gradebook:


These changes will require a simple Devstack restart (make dev.down, make dev.up...). I will note this in my email.

After this PR merges, if a dev has the pre-changes devstack started, then pulls the latest devstack changes, then runs make dev.down, will all containers be properly stopped?

Good point; they will not be stopped until Docker itself or the OS is restarted.

I will include running make dev.down before git-pulling in the email I send out.

kdmccormick · 2021-06-17T13:56:11Z

docker-compose-host.yml

    volumes:
-      - ${DEVSTACK_WORKSPACE}/frontend-app-gradebook:/edx/app/gradebook:cached
-      - gradebook_node_modules:/edx/app/gradebook/node_modules
-      - gradebook_tox:/edx/app/gradebook/.tox


Frontends don't use tox; the gradebook_tox line must have been copypasta. Removing.

kdmccormick · 2021-06-17T13:57:15Z

docker-compose-sync.yml

@@ -59,11 +59,11 @@ volumes:
    external: true
  registrar-sync:
    external: true
-  gradebook-sync:


Yes, several frontends are missing from this file. I'm not going tackle that in this PR. I also doubt that anyone uses the Docker Sync stuff.

Update: sync and NFS are slated for removal soon, so this is even further moot.

docs/decisions/0003-backends-depend-on-frontends.rst

doctoryes

This change makes sense to me - just had a question or two.

doctoryes · 2021-06-21T19:54:08Z

docker-compose-host-nfs.yml

@@ -45,22 +45,22 @@ services:
  registrar-worker:
    volumes:
      - ${DEVSTACK_WORKSPACE}/registrar:/edx/app/registrar/registrar
-  gradebook:
+  frontend-app-gradebook:


After this PR merges, if a dev has the pre-changes devstack started, then pulls the latest devstack changes, then runs make dev.down, will all containers be properly stopped?

docs/decisions/0003-backends-depend-on-frontends.rst

docs/decisions/0005-frontend-package-mounts.rst

davidjoy

This is an approval in spirit more than in detail, as I'm hardly a docker expert.

1. <service>-restart doesn't work for every backend; <service>-restart-devserver is more reliable. 2. <service>-restart-devserver doesn't work on frontends; recommend attach and Ctrl+C instead.

All micro-frontends in Devstack follow the frontend-app-* naming convention except course-authoring, gradebook, and program-console. This was presumably done to reduce typing, but adds confusion via its inconsistency. Furthermore, the upcoming change (in which backend services will depend on MFEs) will make it so these frontend names will be referenced much less often in the typical dev workflow. For example, `make dev.up.lms` will be sufficient to start Gradebook, so typing `make dev.up.frontend-app-gradebook` soon won't be necessary. This is a breaking interface change in that it renames the commands needed to start/stop/manage these frontends; however, the change is not expected to be breaking in any other way.

Diffing / spotting issues in repetative config can be difficult. Alphabetization makes it easier.

It was once separated out because the idea of DEFAULT_SERVICES and a generalized `dev.up.*` command hadn't been developed, so everything in docker-compose.yml was always started. This is no longer the case (the Registrar extra service exists in docker-compose.yml yet isn't started by default). So, the separation of docker-compose-xqueue.yml is just confusing. Furthermore, by pulling xqueue and xqueue_consumer into docker-compose.yml, we can properly define mysql57 as its dependency.

lms now depends on frontend-app-learning; previously, the converse was true. Running `make dev.up.lms` will start the frontend-app-learning container. This change has been made for all Devstack frontends/backends. This is breaking in that `make dev.up.frontend-app-*` is no longer the best way to start a frontend from a clean state, since it will not start related backend service(s). Instead, the backend service should be started, which will cause all related frontend apps to be started as dependencies. See included ADR (#4) for more context. Also includes some doc updates related to this change. TNL-8407

Allows installation of local versions of NPM packages via module.config.js, allowing frontend devs to test out frontend library changes within devstack. The mounts follow nearly the same pattern that micro-services do, which allows devs to develop local versions of Python packages alongside devstack micro-services. See included ADR #5 for details and rationale. TNL-8407

kdmccormick · 2021-08-09T17:37:38Z

Partially reverting this in #815

kdmccormick changed the title ~~invert dependencies so that backends now depend on frontends (+ cleanup)~~ invert dependencies so that backends now depend on frontends (plus some cleanup) Jun 17, 2021

kdmccormick force-pushed the kdmccormick/service-names-and-dependencies branch from bacd16c to e7404a8 Compare June 17, 2021 13:33

kdmccormick commented Jun 17, 2021

View reviewed changes

kdmccormick marked this pull request as ready for review June 17, 2021 14:09

iloveagent57 reviewed Jun 21, 2021

View reviewed changes

docs/decisions/0003-backends-depend-on-frontends.rst Outdated Show resolved Hide resolved

doctoryes reviewed Jun 21, 2021

View reviewed changes

kdmccormick force-pushed the kdmccormick/service-names-and-dependencies branch 2 times, most recently from 298cf9f to 962f6ce Compare June 24, 2021 20:40

kdmccormick mentioned this pull request Jun 25, 2021

feat: mount src/ into /edx/src of frontend containers #790

Closed

kdmccormick force-pushed the kdmccormick/service-names-and-dependencies branch 3 times, most recently from 86779ca to d7bbc3b Compare July 8, 2021 17:27

kdmccormick mentioned this pull request Jul 8, 2021

feat: mount local frontend libs into frontend containers #795

Closed

kdmccormick force-pushed the kdmccormick/service-names-and-dependencies branch from d7bbc3b to ca727ba Compare July 27, 2021 14:16

kdmccormick mentioned this pull request Jul 27, 2021

feat: mount src/ into /edx/app/src of frontend containers #803

Merged

kdmccormick force-pushed the kdmccormick/service-names-and-dependencies branch 2 times, most recently from b49d337 to aa03e4f Compare July 30, 2021 14:35

kdmccormick mentioned this pull request Jul 30, 2021

docs: update docs on local module configuration openedx/frontend-build#193

Merged

kdmccormick force-pushed the kdmccormick/service-names-and-dependencies branch from aa03e4f to c4de2c6 Compare July 30, 2021 15:26

davidjoy reviewed Aug 2, 2021

View reviewed changes

docs/decisions/0005-frontend-package-mounts.rst Outdated Show resolved Hide resolved

davidjoy approved these changes Aug 2, 2021

View reviewed changes

kdmccormick and others added 6 commits August 2, 2021 23:38

docs: minor corrections to devserver-restarting docs

bc41608

1. <service>-restart doesn't work for every backend; <service>-restart-devserver is more reliable. 2. <service>-restart-devserver doesn't work on frontends; recommend attach and Ctrl+C instead.

style: alphabetize service definitions and dependencies

d1844be

Diffing / spotting issues in repetative config can be difficult. Alphabetization makes it easier.

kdmccormick force-pushed the kdmccormick/service-names-and-dependencies branch from cf4be4e to 37a834e Compare August 3, 2021 03:38

kdmccormick merged commit 1b0b65c into master Aug 3, 2021

kdmccormick deleted the kdmccormick/service-names-and-dependencies branch August 3, 2021 17:27

kdmccormick mentioned this pull request Aug 9, 2021

revert: feat!: invert dependencies so that backends now depend on frontends #815

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

invert dependencies so that backends now depend on frontends (plus some cleanup) #783

invert dependencies so that backends now depend on frontends (plus some cleanup) #783

kdmccormick commented Jun 17, 2021 •

edited

Loading

kdmccormick left a comment

kdmccormick Jun 17, 2021

doctoryes Jun 21, 2021

kdmccormick Jun 24, 2021

kdmccormick Jun 17, 2021 •

edited

Loading

kdmccormick Jun 17, 2021

kdmccormick Jul 30, 2021

doctoryes left a comment

doctoryes Jun 21, 2021

davidjoy left a comment

kdmccormick commented Aug 9, 2021

invert dependencies so that backends now depend on frontends (plus some cleanup) #783

invert dependencies so that backends now depend on frontends (plus some cleanup) #783

Conversation

kdmccormick commented Jun 17, 2021 • edited Loading

Description

Supporting information

Testing instructions

Footnotes

kdmccormick left a comment

Choose a reason for hiding this comment

kdmccormick Jun 17, 2021

Choose a reason for hiding this comment

doctoryes Jun 21, 2021

Choose a reason for hiding this comment

kdmccormick Jun 24, 2021

Choose a reason for hiding this comment

kdmccormick Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

kdmccormick Jun 17, 2021

Choose a reason for hiding this comment

kdmccormick Jul 30, 2021

Choose a reason for hiding this comment

doctoryes left a comment

Choose a reason for hiding this comment

doctoryes Jun 21, 2021

Choose a reason for hiding this comment

davidjoy left a comment

Choose a reason for hiding this comment

kdmccormick commented Aug 9, 2021

kdmccormick commented Jun 17, 2021 •

edited

Loading

kdmccormick Jun 17, 2021 •

edited

Loading