You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Michalosu (via GitHub)" <gi...@apache.org> on 2023/08/24 10:49:26 UTC
[GitHub] [airflow] Michalosu opened a new pull request, #33690: SECURITY_MANAGER_CLASS should be a referrence to class, not a string
Michalosu opened a new pull request, #33690:
URL: https://github.com/apache/airflow/pull/33690
See this comment: https://github.com/apache/airflow/issues/33586#issuecomment-1690033003
If SECURITY_MANAGER_CLASS is a string then you will get error like:
```
File "/home/airflow/.local/lib/python3.8/site-packages/airflow/www/extensions/init_appbuilder.py", line 661, in init_appbuilder
if not issubclass(security_manager_class, AirflowSecurityManager):
TypeError: issubclass() arg 1 must be a class
```
@vincbeck fyi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #33690: SECURITY_MANAGER_CLASS should be a referrence to class, not a string
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #33690:
URL: https://github.com/apache/airflow/pull/33690#issuecomment-1691928105
> Exactly, it has been like this for a while, docs were not updated ... for a while too :) No there is no breaking change, just nobody uses that feature I guess :)
ALMOST nobody :) (except @Michalosu ) - or maybe people who used it just figured out how to do it. But yeah, this does not seem like something that is used often. This is also the reason why we want to move it out of Airflow/away from FAB so we can just expose stable API for anyone who would like to develop their custom auth.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vincbeck commented on pull request #33690: SECURITY_MANAGER_CLASS should be a referrence to class, not a string
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #33690:
URL: https://github.com/apache/airflow/pull/33690#issuecomment-1696441955
This issue makes me think ... Should we keep this feature of letting user bringing their own security manager? This feature only made my design/implementation way more complex than without it. When auth managers will be available in Airflow, bringing his own security manager makes no sense to me. And more importantly, it will be impossible to make both world working (the set of auth managers + the set of custom security managers written by users)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vincbeck commented on pull request #33690: SECURITY_MANAGER_CLASS should be a referrence to class, not a string
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #33690:
URL: https://github.com/apache/airflow/pull/33690#issuecomment-1691900107
Exactly, it has been like for a while, docs were not updated ... for a while too :) No there is no breaking change, just nobody uses that feature I guess :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vincbeck commented on pull request #33690: SECURITY_MANAGER_CLASS should be a referrence to class, not a string
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #33690:
URL: https://github.com/apache/airflow/pull/33690#issuecomment-1691806030
Thank you! Nice catch! Could you run static checks? It is currently failing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #33690: SECURITY_MANAGER_CLASS should be a referrence to class, not a string
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #33690:
URL: https://github.com/apache/airflow/pull/33690#issuecomment-1691450185
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (ruff, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
- Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vincbeck merged pull request #33690: SECURITY_MANAGER_CLASS should be a referrence to class, not a string
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck merged PR #33690:
URL: https://github.com/apache/airflow/pull/33690
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #33690: SECURITY_MANAGER_CLASS should be a referrence to class, not a string
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #33690:
URL: https://github.com/apache/airflow/pull/33690#issuecomment-1696444150
> When auth managers will be available in Airflow, bringing his own security manager makes no sense to me.
100% . It should be a feature of FAB Auth Manager only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #33690: SECURITY_MANAGER_CLASS should be a referrence to class, not a string
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #33690:
URL: https://github.com/apache/airflow/pull/33690#issuecomment-1691900687
Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #33690: SECURITY_MANAGER_CLASS should be a referrence to class, not a string
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #33690:
URL: https://github.com/apache/airflow/pull/33690#issuecomment-1691861667
> It would be good to mentioned it in release note of 2.7.0 as this is kinda breaking change if you have own implementation of `AirflowSecurityManager`. Code has been change that you can't use `FAB_SECURITY_MANAGER_CLASS` but docs were still pointing to that and with the wrong example of value.
>
> Good that I know what happened after upgrade to 2.7.0 and created this PR for others if someone else will see this problem - "Boy scout rule"
I think It's been like that for a while already :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #33690: SECURITY_MANAGER_CLASS should be a referrence to class, not a string
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #33690:
URL: https://github.com/apache/airflow/pull/33690#issuecomment-1691841763
Yeah. You can CLEARLY see that that part of Airflow is not getting the love it needs... Hopefully with the AIP-56 we can turn it around and make it more of "ours" so to speak.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Michalosu commented on pull request #33690: SECURITY_MANAGER_CLASS should be a referrence to class, not a string
Posted by "Michalosu (via GitHub)" <gi...@apache.org>.
Michalosu commented on PR #33690:
URL: https://github.com/apache/airflow/pull/33690#issuecomment-1691850708
It would be good to mentioned it in release note of 2.7.0 as this is kinda breaking change if you have own implementation of `AirflowSecurityManager`. Code has been change that you can't use `FAB_SECURITY_MANAGER_CLASS` but docs were still pointing to that and with the wrong example of value.
Good that I know what happened after upgrade to 2.7.0 and created this PR for others if someone else will see this problem - "Boy scout rule"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Michalosu commented on pull request #33690: SECURITY_MANAGER_CLASS should be a referrence to class, not a string
Posted by "Michalosu (via GitHub)" <gi...@apache.org>.
Michalosu commented on PR #33690:
URL: https://github.com/apache/airflow/pull/33690#issuecomment-1691854760
> Thank you! Nice catch! Could you run static checks? It is currently failing
Executed pre-commit locally for that file and it is corrected, waiting for results from CI and looking forward for first contribution to airflow repository :1st_place_medal:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Michalosu commented on pull request #33690: SECURITY_MANAGER_CLASS should be a referrence to class, not a string
Posted by "Michalosu (via GitHub)" <gi...@apache.org>.
Michalosu commented on PR #33690:
URL: https://github.com/apache/airflow/pull/33690#issuecomment-1691878120
To be honest, I don't see it directly in https://airflow.apache.org/docs/apache-airflow/stable/release_notes.html, "grepping" for `FAB_SECURITY_MANAGER_CLASS` and `AirflowSecurityManager` but perhaps I'm blind, never mind :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SECURITY_MANAGER_CLASS should be a referrence to class, not a string [airflow]
Posted by "andrew-candela (via GitHub)" <gi...@apache.org>.
andrew-candela commented on PR #33690:
URL: https://github.com/apache/airflow/pull/33690#issuecomment-1958505840
Just ran into this issue when trying to upgrade from 2.6 to 2.8. Thanks for updating the docs @Michalosu !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org