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

Changes in 0.15.x to use kolibri with external plugins #9543

Merged
merged 3 commits into from Jul 5, 2022

Conversation

jredrejo
Copy link
Member

References

Testing kolibri 0.15 as base for the KDP plugin I've found some blockers that need changes in the current kolibri code.
With these changes the backend now can run, in the future there might be also needed changes for the frontend.
In summary the changes are:

  • In migrations, replace settings.AUTH_USER_MODEL by the kolibriauth.FacilityUser model because when kolibri is run in a different environment the AUTH_USER_MODEL can be different and migrations will break.
  • A small change to the Django initialization process to bear in mind other possible cache backends.

Reviewer guidance

  • Do tests pass
  • Any possible regression?

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

@jredrejo jredrejo added the TODO: needs review Waiting for review label Jun 29, 2022
@marcellamaki marcellamaki added this to the Planned Patch 4: Coach usability improvements milestone Jun 29, 2022
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.

Some comments - nothing blocking. Also suggested adding a catch to the device migration that causes issues when there's a DB cache.

@@ -15,7 +15,7 @@ class Migration(migrations.Migration):

dependencies = [
("kolibriauth", "0019_collection_no_mptt"),
migrations.swappable_dependency(settings.AUTH_USER_MODEL),
migrations.swappable_dependency("kolibriauth.FacilityUser"),
Copy link
Member

Choose a reason for hiding this comment

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

Oh wow, it's Django doing this - in the model specification it's explicitly targeting the FacilityUser model, but it went ahead and turned it into this reference anyway: https://github.com/learningequality/kolibri/blob/develop/kolibri/core/bookmarks/models.py#L18

@@ -236,6 +236,10 @@ def _post_django_initialization():
settings.CACHES["process_cache"]["TIMEOUT"],
**settings.CACHES["process_cache"]["OPTIONS"]
)
except AttributeError:
Copy link
Member

Choose a reason for hiding this comment

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

I guess the other way of handling this would be to check if the process_cache is not the default cache - which in the case of Redis it is, and I assume for online instances also?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand this comment. If process_cache is in CACHES (i.e. it's not the default cache) it means we're not using redis.
The logic would be mostly the same. We can remove the attribute error checking the cache BACKEND. Is that what you have in mind? :


def _post_django_initialization():
    from kolibri.deployment.default.cache import CACHES

    if 'process_cache' in CACHES:
        backend = CACHES['process_cache']['BACKEND']
        if backend != 'django.core.cache.backends.db.DatabaseCache':
            try:
                process_cache.cull()
            except SQLite3DatabaseError:
                shutil.rmtree(process_cache.directory, ignore_errors=True)
                os.mkdir(process_cache.directory)
                process_cache._cache = FanoutCache(
                    process_cache.directory,
                    settings.CACHES["process_cache"]["SHARDS"],
                    settings.CACHES["process_cache"]["TIMEOUT"],
                    **settings.CACHES["process_cache"]["OPTIONS"]
                )


Copy link
Member

Choose a reason for hiding this comment

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

Yeah, mostly just to make it based on Django settings, rather than our OPTIONS, so in the case that it's overridden by a custom settings file, we're still checking properly.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, done it. I have also added a guard in kolibri/core/device/migrations/0003_contentcachekey.py to avoid the problems this migration had when running in a system using DatabaseCache.

@jredrejo jredrejo modified the milestones: Planned Patch 4: Coach usability improvements, Planned Patch 5 Jul 5, 2022
@rtibbles rtibbles merged commit 953d32a into learningequality:release-v0.15.x Jul 5, 2022
@jredrejo jredrejo deleted the 015_for_kdp branch July 6, 2022 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants