fix: convert variables that replace mfe .env into string #202
Conversation
felipemontoya
force-pushed
the
fmo/mfe_config_str_values
branch
from
27e6255 to
9359a3f
Compare
| @@ -72,7 +72,7 @@ MFE_CONFIG["ACCOUNT_PROFILE_URL"] = "{% if ENABLE_HTTPS %}https://{% else %}http | |||
|
|
|||
| {% if get_mfe("communications") %} | |||
| COMMUNICATIONS_MICROFRONTEND_URL = "{% if ENABLE_HTTPS %}https://{% else %}http://{% endif %}{{ MFE_HOST }}/communications" | |||
| MFE_CONFIG["SCHEDULE_EMAIL_SECTION"] = True | |||
| MFE_CONFIG["SCHEDULE_EMAIL_SECTION"] = "true" | |||
There was a problem hiding this comment.
Why are we replacing bool to string here? We are not comparing anywhere it with "true" here.
There was a problem hiding this comment.
Thanks a lot for the review. I changed all the bools with strings in this PR because they would all trip with the same issue with process.env returning strings and MFE_CONFIG returning bool.
I might have overreached as I did not test all of them.
Given the latest round of comments in the course-authoring PR I think we might close this PR altogether. I'll update this PR after we settle the discussion there.
There was a problem hiding this comment.
I have gone through the course-authoring discussion and just got that the issue was in Quince 3. Is that resolved now so I can close this PR?
There was a problem hiding this comment.
The reason for replacing all bool to strings is that this would be the theoretically correct behavior for all MFE_CONFIG variables. I'll reproduce what I wrote on another PR:
getConfig()still returns values as they were set by the .env files, unless they were overriden by runtime config. Since .env files are still supported, and the dotenv package that reads them in doesn't support booleans, we have no choice but to assume everything thatgetConfig()spits out is a string.
The problem is that some MFEs don't follow this assumption correctly. For instance, to use your example search, take a look here:
This line is actually wrong. The correct check would be getConfig().SCHEDULE_EMAIL_SECTION === "true". What it's actually doing now is just checking to see if the variable is empty of not. Setting it to "False" in a .env file would not have the expected results.
All of this said, we'll just have to live with some inconsistencies in tutor-mfe until everything conforms to the same standard upstream.
| @@ -28,7 +28,7 @@ MFE_CONFIG = { | |||
| {% if get_mfe("authn") %} | |||
| AUTHN_MICROFRONTEND_URL = "{% if ENABLE_HTTPS %}https://{% else %}http://{% endif %}{{ MFE_HOST }}/authn" | |||
| AUTHN_MICROFRONTEND_DOMAIN = "{{ MFE_HOST }}/authn" | |||
| MFE_CONFIG["DISABLE_ENTERPRISE_LOGIN"] = True | |||
| MFE_CONFIG["DISABLE_ENTERPRISE_LOGIN"] = "true" | |||
There was a problem hiding this comment.
Same here. Can you please share why are we converting to string here as well? I checked this here. There is no comparison done against a string.
|
Given that openedx/frontend-app-authoring#951 was merged with a different approach and it removed one of the keys I was modifiying in this PR, I think the best course of action for this PR is to close it. |
This PR converts values from Boolean to String in the cases where the value replaces
.envvalues used in MFEs.It is one of the options for solving openedx/frontend-app-authoring#835