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 loading of periodic tasks and clean up extension loading. #4064

Merged
merged 3 commits into from
Aug 13, 2019

Conversation

jezdez
Copy link
Member

@jezdez jezdez commented Aug 12, 2019

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other

Description

This does a few things:

  • add tests for extension loading
  • actually call the periodic tasks entry point callback instead of using the function to pass to Celery (regression!)
  • refactor the extension and periodic task loading
  • better handle assertions raised by extensions (e.g. when an extension tries to override an already registered view)
  • attach exception traceback to error log during loading for improved debugging

@jezdez jezdez requested a review from arikfr August 12, 2019 12:10
@jezdez
Copy link
Member Author

jezdez commented Aug 12, 2019

@arikfr Hey, just wanted to say that this is something we really need to ship in v8 since the periodic task loading mechanism was simply broken last time we touched the code. I forgot to not only load the entrypoints but also call the them to get the schedule parameters to be passed to Celery.

This does a few things:

- add tests for extension loading
- refactor the extension and periodic task loading
- better handle assertions raised by extensions (e.g. when an extension tries to override an already registered view)
- attach exception traceback to error log during loading for improved debugging
@jezdez jezdez force-pushed the improved-extensions branch from b4b93a5 to 56ff952 Compare August 12, 2019 12:22
Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

💯 on adding tests. Can you see why they fail in the CI build?

@jezdez
Copy link
Member Author

jezdez commented Aug 12, 2019

Hm, the tests pass locally, it's weird. Must be a difference in environment since it calls pip to actually install the dummy package. Let me see if I can figure out a different way to ship the dummy package.

@arikfr
Copy link
Member

arikfr commented Aug 12, 2019

Did the local tests work inside a container?

@jezdez
Copy link
Member Author

jezdez commented Aug 12, 2019

@arikfr Oh yeah, totally. I've been running them locally in the regular Docker dev environment.

I just pushed a change to use site.addsitedir instead of calling pip, let's see if this fixes it.

@jezdez
Copy link
Member Author

jezdez commented Aug 12, 2019

Update: Yep, using site.addsitedir fixed it. I haven't a clue why calling pip didn't work on Circle.

@jezdez
Copy link
Member Author

jezdez commented Aug 12, 2019

For the record, I created the egg-info directory (which is what is created in the site-packages directory when running pip install <packagename>) by running:

  • make bash to create a shell inside the container
  • cd tests/extensions
  • pip install --user --editable redash-dummy/ to install the package in the user's "local" directory (~/.local) to work around the fact the containers runs as an unprivileged user
  • pip uninstall --yes redash-dummy to simply remove it again

The egg-info directory is not cleaned up by pip, just the link in the ~/.local site-packages directory.

@jezdez jezdez requested a review from arikfr August 12, 2019 13:39
Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

How about adding the instructions on how to create the egg-info directory to a README file in the extension directory?

tests/extensions/test_extensions.py Outdated Show resolved Hide resolved
@arikfr arikfr merged commit 7b5696d into getredash:master Aug 13, 2019
@arikfr
Copy link
Member

arikfr commented Aug 13, 2019

👍

@jezdez jezdez deleted the improved-extensions branch August 13, 2019 11:34
harveyrendell pushed a commit to pushpay/redash that referenced this pull request Nov 14, 2019
…ash#4064)

* Fix loading of periodic tasks and clean up extension loading.

This does a few things:

- add tests for extension loading
- refactor the extension and periodic task loading
- better handle assertions raised by extensions (e.g. when an extension tries to override an already registered view)
- attach exception traceback to error log during loading for improved debugging

* Use site.addsitedir instead of calling pip.

* Use sys.path instead of site.addsitedir and also the setup.py egg_info command.
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.

2 participants