Skip to content

Conversation

@lepture
Copy link

@lepture lepture commented Sep 18, 2019

Make sure you have checked all steps below.

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:

Flask-OAuthlib is deprecated, use Authlib instead. Updated Google and GitHub Enterprise OAuth backends.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Flask-OAuthlib is deprecated, use Authlib instead.
@codecov-io
Copy link

codecov-io commented Sep 18, 2019

Codecov Report

Merging #6140 into master will increase coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6140      +/-   ##
==========================================
+ Coverage   80.02%   80.02%   +<.01%     
==========================================
  Files         607      607              
  Lines       35032    35041       +9     
==========================================
+ Hits        28034    28042       +8     
- Misses       6998     6999       +1
Impacted Files Coverage Δ
airflow/contrib/auth/backends/google_auth.py 0% <0%> (ø) ⬆️
...ow/contrib/auth/backends/github_enterprise_auth.py 0% <0%> (ø) ⬆️
airflow/utils/dag_processing.py 58.8% <0%> (-0.19%) ⬇️
airflow/jobs/backfill_job.py 91.43% <0%> (+1.52%) ⬆️
airflow/jobs/local_task_job.py 90% <0%> (+5%) ⬆️

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 7ad1544...1016311. Read the comment docs.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

I like the change. What worries me a bit is that there are no tests covering the functionality. I don't think there are tests covering the original oauthlib but maybe that's an opportunity to introduce some tests - mocking the behaviour of the oauth ?

@@ -216,9 +216,7 @@ def write_version(filename: str = os.path.join(*["airflow", "git_version"])):
]
grpc = ['grpcio>=1.15.0']
flask_oauth = [
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be renamed to "oauth" :)

Copy link
Author

Choose a reason for hiding this comment

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

Can I change this name? It was named flask_oauth.

@lepture
Copy link
Author

lepture commented Sep 19, 2019

@potiuk It will take me some time to figure out how to write tests for airflow. Maybe in next week.

@feluelle feluelle added the area:webserver Webserver related Issues label Sep 20, 2019
@potiuk
Copy link
Member

potiuk commented Oct 14, 2019

Hey @lepture - will you have chance to work on it soon :) ?

@lepture
Copy link
Author

lepture commented Oct 14, 2019

@potiuk I'm afraid that I don't have time right now to figure out how to write the tests.

@potiuk
Copy link
Member

potiuk commented Oct 14, 2019

Ok. I see and understand. Maybe I will try to take a look at that then :).

@lepture
Copy link
Author

lepture commented Oct 14, 2019

@potiuk if there is an example that would help a lot. I can't find any similar tests.

@stale
Copy link

stale bot commented Nov 28, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 28, 2019
@stale stale bot closed this Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:webserver Webserver related Issues stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants