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

[Agrendalath]

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:

• Have a Site Reliability Engineer review the PR if you don't own all of the services impacted.
• If you are adding any new default values that need to be overridden when this change goes live, update internal repos and add an entry to the top of the CHANGELOG.
• Performed the appropriate testing.
• Think about how this change will affect Open edX operators and update the wiki page for the next Open edX release if needed

[openedx-webhooks]

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:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

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.

[pomegranited]

@Agrendalath Why can't you use MFE_DEPLOY_ENVIRONMENT_EXTRA when defining extra variables for all MFEs? That's what the mfe_deployer README recommends.

[Agrendalath]

@pomegranited, ah, I made a typo in the description. I replaced MFE_ENVIRONMENT_EXTRA with MFE_DEPLOY_ENVIRONMENT_EXTRA now.

As you can see below, this variable is used with a default filter, which means that any env_extra defined for an MFE will override these values:
https://github.com/openedx/configuration/blob/1909bafde44584ec7bd04f7cb6ee6ac0bfa6b521/playbooks/roles/mfe_deployer/tasks/main.yml#L16

I described a way to fix this in Alternative 2, along with my concerns about this being backward-incompatible.

If you meant using the | combine(MFE_DEPLOY_ENVIRONMENT_EXTRA) variable in the highlighted line (i.e., instead of the newly added MFE_ENVIRONMENT_DEFAULT_EXTRA), this is similar to Alternative 3 (the only difference is that we would not replace MFE_ENVIRONMENT_EXTRA with MFE_DEPLOY_ENVIRONMENT_EXTRA directly in the mfe_deployer role, but add an extra combine filter mentioned above to the mfe role). It would work, but it wouldn't be backward-compatible.

These solutions would be much better (and more straightforward) if we did not care about compatibility. However, I decided to keep this safe by adding a new variable that does not alter how this role currently works. If we knew nobody relied on this behavior, I would be happy to change it.

[pomegranited]

Ok, I see the difference now, thank you for explaining!

[pomegranited]

👍

I am reluctant to add even more variables to the MFE/deployer role, but I agree that this is the best solution to the issue described.

Will merge this tomorrow if there are no objections. CC @mphilbrick211

• I tested this by checking that the MFE env variables of a deployed MFE service included both those defined in MFE_ENVIRONMENT_DEFAULT_EXTRA and those defined in the specific MFE's env_extra.
• I read through the code and checked usages and comments for the new variable.
• I checked for accessibility issues N/A
• Includes documentation -- see CHANGELOG and comments.
• User-facing strings are extracted for translation N/A

[openedx-webhooks]

@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.