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

feat: new patch for mysql options #1155

Closed

Conversation

kaustavb12
Copy link
Contributor

Description

Currently no charset is set if extrenal MySQL instance is used with Tutor. Also, there is no option to set charset as required.

This PR introduces a patch called mysql-options which will allow tutor user to set charset and other options as required.

Other information

Private Ref : FAL-3945

@bradenmacdonald
Copy link
Contributor

Shouldn't we just make charset: "utf8mb4" the default in general, rather than relying on users to patch it? Is it not the case because some people use an old version of MySQL that doesn't support it, or have old table formats, or what?

@kaustavb12
Copy link
Contributor Author

@bradenmacdonald

Making the charset default would definitely be the easier option.

Is it not the case because some people use an old version of MySQL that doesn't support it, or have old table formats, or what?

Yes, I suspect that is why the {%- if RUN_MYSQL %} condition was added in the first place.

I debated whether to just remove the if condition, but finally went with adding the new patch since that would give more flexibility anyways for adding other options there if someone needs it.

@bradenmacdonald
Copy link
Contributor

We'll need some input here from a Tutor maintainer.

@@ -20,5 +20,6 @@ DATABASES:
{%- if RUN_MYSQL %}
charset: "utf8mb4"
{%- endif %}
{{ patch("mysql-options") }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not add anything to this file, which I would very much like to see disappear. MySQL settings can already be overridden from the edx-platform common settings. Let's not add useless patches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. Closing this PR.

@kaustavb12 kaustavb12 closed this Nov 20, 2024
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