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

Fix to Env Var and Config persisting in options.ini in KOLIBRI_HOME #11737

Conversation

thesujai
Copy link
Contributor

@thesujai thesujai commented Jan 17, 2024

Summary

This Solution Aims to override the default config of environment variables in _set_from_envvars

References

Fixes #9551

Reviewer guidance


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: very small labels Jan 17, 2024
@thesujai
Copy link
Contributor Author

Hello @rtibbles, please also have a look at 556de59, this way the DEBUG and other env variables that are generated persist(ephemeral configuration), but they are set to FALSE

@thesujai thesujai changed the title override the configuration to default false Fix to Env Var and Config persisting in options.ini in KOLIBRI_HOME Jan 17, 2024
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

This breaks existing functionality, even if it does fix the issue.

kolibri/utils/options.py Outdated Show resolved Hide resolved
@rtibbles rtibbles self-assigned this Jan 17, 2024
@thesujai
Copy link
Contributor Author

@rtibbles I made a change directly in the generate_empty_options_file so i dont think it will break any functionality, as it is supposed to run only in initialization process, that too only when the options.ini not already exists (https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/utils/sanity_checks.py#L138)

So ig this should not break any functionality and still solve the issue

@thesujai thesujai requested a review from rtibbles January 18, 2024 09:23
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Excellent! This does exactly what we need, with a very small diff. Code looks good, and manual testing checks out.

@rtibbles
Copy link
Member

Docs build failure is fixed on the release branch, merging.

@rtibbles rtibbles merged commit 1f3a554 into learningequality:release-v0.16.x Jan 19, 2024
33 of 34 checks passed
@thesujai thesujai deleted the env-variable-persisting-in-option.ini branch January 19, 2024 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: backend Python, databases, networking, filesystem... SIZE: very small
Projects
None yet
2 participants