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

Remove wrapping quotes from env settings #56

Merged
merged 1 commit into from
Jul 11, 2022
Merged

Remove wrapping quotes from env settings #56

merged 1 commit into from
Jul 11, 2022

Conversation

jcohen28
Copy link
Contributor

Prior versions of docker-compose had a bug that did not properly escape quotation marks in env_files, incorrectly including them as characters in the string value. The issue was fixed in docker-compose version 1.26.0. If tutor is run on a system that uses an earlier version of docker-compose, then tutor-mfe functionality gets broken by invalid config values.

This should hopefully avoid the issues discussed here: https://discuss.openedx.org/t/mfe-failing-login-refresh-with-404-error/7716/6

@regisb regisb requested a review from arbrandes July 11, 2022 14:01
Copy link
Collaborator

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

Makes sense. Good catch.

@arbrandes arbrandes merged commit 0b695b9 into overhangio:master Jul 11, 2022
@jcohen28 jcohen28 deleted the docker-quotes branch July 11, 2022 14:28
ghassanmas added a commit to ghassanmas/tutor-mfe that referenced this pull request Sep 13, 2022
  tutor-mfe set SITE_NAME env variable of mfe to equal
  PLATFORM_NAME config value, however in case it has a space, this
  would result SITE_NAME to be empty.
  This change prefix space with \ as, i.e My Site becaomse
  My\ Site.
  Before this might not has been issued because all values were
  wrapped with ' hence overhangio#56

  To test this change (you have to be in production mode)
  - Set your PLATFORM_NAME to any thing that incldues space
  - Build the mfe image
  - Check `document.title` in dev console of any mfe
  - It will not incldue PLATFORM_NAME

  After applying this patch, the last step above, should result
  the PLATFORM_NAME i.e. "Learning | PLATFORM_NAME"
regisb pushed a commit that referenced this pull request Oct 3, 2022
  tutor-mfe set SITE_NAME env variable of mfe to equal
  PLATFORM_NAME config value, however in case it has a space, this
  would result SITE_NAME to be empty.
  This change prefix space with \ as, i.e My Site becaomse
  My\ Site.
  Before this might not has been issued because all values were
  wrapped with ' hence #56

  To test this change (you have to be in production mode)
  - Set your PLATFORM_NAME to any thing that incldues space
  - Build the mfe image
  - Check `document.title` in dev console of any mfe
  - It will not incldue PLATFORM_NAME

  After applying this patch, the last step above, should result
  the PLATFORM_NAME i.e. "Learning | PLATFORM_NAME"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants