openedx-unsupported / configuration

A collection of edx configuration scripts and utilities that edx.org uses to deploy openedx.
GNU Affero General Public License v3.0
823 stars 969 forks source link

fix: allow defining MFE-specific env overrides while preserving extra defaults #7090

Closed Agrendalath closed 10 months ago

Agrendalath commented 10 months ago

User story

When deploying MFEs through the mfe_deployer role (used in the openedx_native.yml playbook), you can specify the following configuration:

MFES:
  - name: gradebook
    repo: frontend-app-gradebook
    public_path: /gradebook/

Then, you define some common env variables that will be shared between your MFEs, like:

MFE_DEPLOY_ENVIRONMENT_EXTRA:
  ACCOUNT_PROFILE_URL: /profile
  ACCOUNT_SETTINGS_URL: '{{ COMMON_LMS_BASE_URL }}/account/settings'

However, now you want to use the MFE config API, as some of the MFE variables are not populated at build time. You also want to use overrides for individual MFEs (with /api/mfe_config/v1?mfe=name_of_mfe), but it doesn't work. That's because the APP_ID is not defined anywhere (so the MFE sends queries to /api/mfe_config/v1?mfe=undefined).

To avoid this (without making further modifications to the MFE env defaults or the mfe_deployer role), you could use the following config:

MFES:
  - name: gradebook
    repo: frontend-app-gradebook
    public_path: /gradebook/
    env_extra:
      APP_ID: gradebook

However, after the deployment, you notice that none of the variables defined in MFE_DEPLOY_ENVIRONMENT_EXTRA were used when running npm build for each MFE. That's because the mfe_deployer role overrides the whole MFE_DEPLOY_ENVIRONMENT_EXTRA variable with anything you define in env_extra: https://github.com/openedx/configuration/blob/1909bafde44584ec7bd04f7cb6ee6ac0bfa6b521/playbooks/roles/mfe_deployer/tasks/main.yml#L16

To mitigate this, I added MFE_ENVIRONMENT_DEFAULT_EXTRA, which you can use in Ansible variables (instead of MFE_DEPLOY_ENVIRONMENT_EXTRA) to share env variables between MFEs, while still allowing to use env_extra for each of them.

Alternatives

  1. Technically, you don't need to make any changes if you overwrite MFE_ENVIRONMENT_DEFAULT in your Ansible vars, but you would need to duplicate all these helpful defaults, which is not an elegant approach.
  2. I thought about changing default to combine in this line, but this would be a breaking change because it alters the current behavior of the Ansible roles.
  3. I also considered changing MFE_ENVIRONMENT_EXTRA to something like MFE_DEPLOYER_CUSTOM_MFE_ENVIRONMENT here, but this is also a breaking change for anybody who relies on the existing behavior.

Author's note

I know the APP_ID is not the best example here, as we could modify the mfe_deployer role to populate this value automatically. We have a slightly more complex use case for the frontend-app-learner-portal-enterprise, but I wanted to keep the user story simple.

Private-ref: BB-8397

Configuration Pull Request

Make sure that the following steps are done before merging:

openedx-webhooks commented 10 months ago

Thanks for the pull request, @Agrendalath! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

openedx-webhooks commented 10 months ago

@Agrendalath 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.