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

chore: Migrating reports to AuthWebdriverProxy #10567

Merged
merged 31 commits into from
Aug 12, 2020

Conversation

craig-rueda
Copy link
Member

@craig-rueda craig-rueda commented Aug 10, 2020

SUMMARY

This PR merges the functionality found in the reports scheduling Webdriver creation with the logic found in Thumbnails. The Thumbnails version allows for option overriding where necessary and is in general superior to that found in scheduling.

  • Refactors reporting to leverage shared Webdriver
  • Adds hook for overriding get_auth_cookies(User) method via MACHINE_AUTH_PROVIDER_CLASS config option
  • Introduces new MachineAuthProviderFactory to manage auth provider

@craig-rueda craig-rueda changed the title Migrating reports to AuthWebdriverProxy chore: Migrating reports to AuthWebdriverProxy Aug 10, 2020
@craig-rueda craig-rueda marked this pull request as ready for review August 11, 2020 01:04
@craig-rueda craig-rueda reopened this Aug 11, 2020
@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2020

Codecov Report

Merging #10567 into master will decrease coverage by 4.25%.
The diff coverage is 74.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10567      +/-   ##
==========================================
- Coverage   63.97%   59.71%   -4.26%     
==========================================
  Files         768      770       +2     
  Lines       36357    36368      +11     
  Branches     3431     3431              
==========================================
- Hits        23259    21718    -1541     
- Misses      12985    14458    +1473     
- Partials      113      192      +79     
Flag Coverage Δ
#cypress ?
#javascript 59.87% <ø> (ø)
#python 59.62% <74.34%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/utils/webdriver.py 54.05% <54.05%> (ø)
superset/utils/screenshots.py 36.36% <80.00%> (+5.48%) ⬆️
superset/tasks/schedules.py 79.52% <91.66%> (-1.08%) ⬇️
superset/utils/machine_auth.py 94.11% <94.11%> (ø)
superset/app.py 80.95% <100.00%> (+0.22%) ⬆️
superset/config.py 90.34% <100.00%> (+0.11%) ⬆️
superset/extensions.py 95.65% <100.00%> (+0.09%) ⬆️
superset/tasks/thumbnails.py 43.33% <100.00%> (ø)
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
... and 156 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a819797...2fef6c5. Read the comment docs.

@@ -0,0 +1,190 @@
# Licensed to the Apache Software Foundation (ASF) under one
Copy link
Member Author

Choose a reason for hiding this comment

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

Moving all things webdriver into a central location

Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

Looks good, but tests need changes

tests/util/webdriver_tests.py Outdated Show resolved Hide resolved
tests/util/webdriver_tests.py Outdated Show resolved Hide resolved
tests/util/webdriver_tests.py Outdated Show resolved Hide resolved
tests/util/webdriver_tests.py Outdated Show resolved Hide resolved
Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

Looks very good. A blocker on non related run.sh. And a couple nice to haves/check it if works

}

#
# Init global vars
#
DB_NAME="test"
DB_NAME="superset"
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct, the intend here is to use a separate DB and having a simple way to DROP/CREATE it at will

superset/config.py Show resolved Hide resolved
superset/utils/machine_auth.py Show resolved Hide resolved
superset/utils/webdriver.py Outdated Show resolved Hide resolved
superset/utils/webdriver.py Outdated Show resolved Hide resolved
superset/utils/webdriver.py Outdated Show resolved Hide resolved
superset/utils/webdriver.py Show resolved Hide resolved
def auth(self, user: "User") -> WebDriver:
driver = self.create()
# Setting cookies requires doing a request first
driver.get(headless_url("/login/"))
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this from here and place it inside machine_auth_provider_factory.instance.authenticate_webdriver(driver, user)

Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

LGTM, we have a couple of breaking changes here, so let's update UPDATING.md. Check if we need to update docs also.

@bkyryliuk I think your review would be very useful here

@dpgaspar dpgaspar self-requested a review August 12, 2020 18:43
@craig-rueda craig-rueda merged commit 2aaa4d9 into apache:master Aug 12, 2020
@craig-rueda craig-rueda deleted the fix/combine-webdrivers branch August 12, 2020 20:28
Ofeknielsen pushed a commit to ofekisr/incubator-superset that referenced this pull request Oct 5, 2020
* Migrating reports to AuthWebdriverProxy

* Extracting out webdriver proxy / Adding thumbnail tests to CI

* Adding license

* Adding license again

* Empty commit

* Adding thumbnail tests to CI

* Switching thumbnail test to Postgres

* Linting

* Adding mypy:ignore / removing thumbnail tests from CI

* Putting ignore statement back

* Updating docs

* First cut at authprovider

* First cut at authprovider mostly working - still needs more tests

* Auth provider tests added

* Linting

* Linting again...

* Linting again...

* Busting CI cache

* Reverting workflow change

* Fixing dataclasses

* Reverting back to master

* linting?

* Reverting installation.rst

* Reverting package-lock.json

* Addressing feedback

* Blacking

* Lazy logging strings

* UPDATING.md note
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* Migrating reports to AuthWebdriverProxy

* Extracting out webdriver proxy / Adding thumbnail tests to CI

* Adding license

* Adding license again

* Empty commit

* Adding thumbnail tests to CI

* Switching thumbnail test to Postgres

* Linting

* Adding mypy:ignore / removing thumbnail tests from CI

* Putting ignore statement back

* Updating docs

* First cut at authprovider

* First cut at authprovider mostly working - still needs more tests

* Auth provider tests added

* Linting

* Linting again...

* Linting again...

* Busting CI cache

* Reverting workflow change

* Fixing dataclasses

* Reverting back to master

* linting?

* Reverting installation.rst

* Reverting package-lock.json

* Addressing feedback

* Blacking

* Lazy logging strings

* UPDATING.md note
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XL 🚢 0.38.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants