-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
AIP-81 Include CLI Token Expiration Time API Configuration and Integrate New Config in SimpleAuthManager #46839
AIP-81 Include CLI Token Expiration Time API Configuration and Integrate New Config in SimpleAuthManager #46839
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I assume then you want to use the simple auth manager in the CLI? That's cool :)
35f0584
to
320daae
Compare
That is exactly my intention :) Thanks for the review! Fixed the openapi specs. Surprised it got away from my local pre-commit run before the commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add unit tests?
For sure, I am going to add them. Sorry for double-checking! I am going to work on the changes needed in FAB next for this integration. I might again need your review there :) |
For sure! Happy to review :) Feel free to add me as reviewer/tag me |
320daae
to
9f69120
Compare
…ce to decide and auth with simple auth manager to enable further development
Thanks a lot! I will do that :) I added unit tests and moved some parts to |
9f69120
to
758ac9b
Compare
Sorry I did not add comments as part of one review, they arrived in my mind after my first review :) |
All welcome! Thanks! I'm going to check in a moment :) |
…and route test logic and include tests
I addressed them all. Whenever you have time :) |
airflow/auth/managers/simple/services/simple_auth_manager_login.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for addressing all the comments
Thanks for your time! :) |
…ate New Config in SimpleAuthManager (apache#46839)
closes: #46837
I also included the token generation in SimpleAuthManager to enable further development.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.