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

Add [webserver]update_fab_perms to deprecated configs #40317

Merged

Conversation

ephraimbuddy
Copy link
Contributor

[webserver]update_fab_perms is deprecated in favour of [fab]update_fab_perms and has been a breaking change since 2.9.0. This PR adds the config to the deprecated config list to properly inform users and have both options work at the moment

@ashb
Copy link
Member

ashb commented Jun 19, 2024

I wonder if we should also maybe the config in the fab provider look at the the old config as well?

It's fairly niche so I don't mind either way if we decide to leave the provider looking only at the new option

@ashb
Copy link
Member

ashb commented Jun 19, 2024

An alternative (that I don't think I like, but it is an option) is to mutate configuration.ConfigParser.deprecated_options to add the FAB option from within the FAB provider itself. That feels icky though

@ephraimbuddy
Copy link
Contributor Author

An alternative (that I don't think I like, but it is an option) is to mutate configuration.ConfigParser.deprecated_options to add the FAB option from within the FAB provider itself. That feels icky though

I moved the provider config loading into AppBuilder init section. Looks like it makes sense that way

@ephraimbuddy ephraimbuddy force-pushed the add-dep-webserver-update-fab-perms branch from 49c6d9c to eccd32a Compare June 19, 2024 14:03
@ephraimbuddy ephraimbuddy requested a review from kaxil as a code owner June 20, 2024 06:48
@ephraimbuddy
Copy link
Contributor Author

I had to let the doc builder build FAB configuration to fix the doc configuration issue. I haven't figured a better way to fix it cc @potiuk. The doc issue : https://github.com/apache/airflow/actions/runs/9585199812/job/26430786160?pr=40317#step:7:14376

@potiuk
Copy link
Member

potiuk commented Jun 20, 2024

The first two errors were causing all the others @ephraimbuddy - apparently somewhere there is a reference to config:fab__auth_rate_limit and this reference is gone when you move configuration to fab provider - it should point to fab provider configuration instead using intersphinx references

------------------------------ Error   2 --------------------
 WARNING: undefined label: 'config:fab__auth_rate_limit'

File path: /opt/airflow/docs/apache-airflow/<unknown>
------------------------------ Error   3 --------------------
 WARNING: undefined label: 'config:fab__update_fab_perms'

@ephraimbuddy
Copy link
Contributor Author

ephraimbuddy commented Jun 20, 2024

The first two errors were causing all the others @ephraimbuddy - apparently somewhere there is a reference to config:fab__auth_rate_limit and this reference is gone when you move configuration to fab provider - it should point to fab provider configuration instead using intersphinx references

------------------------------ Error   2 --------------------
 WARNING: undefined label: 'config:fab__auth_rate_limit'

File path: /opt/airflow/docs/apache-airflow/<unknown>
------------------------------ Error   3 --------------------
 WARNING: undefined label: 'config:fab__update_fab_perms'

I can't find the references of those configs. The issue seems to come from having ("fab", "auth_rate_limit"), etc in deprecations while there's no reference to the fab section in core airflow documentation. The docs build fine when the items are removed from the deprecation sections. The provided solution of having the fab section in core config ref also solved this.
cc @potiuk

@potiuk
Copy link
Member

potiuk commented Jun 20, 2024

I can't find the references of those configs. The issue seems to come from having ("fab", "auth_rate_limit"), etc in

This is the issues-> those deprecations will automatically generate links to the new parameters like for example here:

Screenshot 2024-06-20 at 16 38 00

And this is where it fails, because they cannot be found as they are now generated in fab provider module not in airflow - I think it's worth checking how generated documentation looks like and whether it does not cause duplication for those deprecated parameter descriptions.

@potiuk
Copy link
Member

potiuk commented Jun 20, 2024

Yes. As I thought. What happens now is that generated documentation is wrong:

Fab parameters are present in "airflow" docs

Screenshot 2024-06-20 at 16 49 29

But also FAB parameters are missing in fab provider docs (also in "production" documentation).

Screenshot 2024-06-20 at 16 58 23

(only auth_rate_limit is present).

After a closer look - it looks like simply version_added is set wrongly in FAB's provider.yaml

Screenshot 2024-06-20 at 16 59 08

And actually fixing versions there should be the only thing needed to fix the problem.

`[webserver]update_fab_perms` is deprecated in favour of `[fab]update_fab_perms` and has been a breaking change since 2.9.0. This PR adds the config to the deprecated config list to properly inform users and have both options work at the moment
ephraimbuddy and others added 2 commits June 20, 2024 20:42
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
@ephraimbuddy ephraimbuddy force-pushed the add-dep-webserver-update-fab-perms branch from 99c6970 to 5c53c3a Compare June 20, 2024 19:42
@ephraimbuddy
Copy link
Contributor Author

Yeah, that solved it. Thanks so much @potiuk

@ephraimbuddy ephraimbuddy merged commit e24b7c1 into apache:main Jun 20, 2024
52 checks passed
@ephraimbuddy ephraimbuddy deleted the add-dep-webserver-update-fab-perms branch June 20, 2024 21:45
@utkarsharma2 utkarsharma2 added the type:bug-fix Changelog: Bug Fixes label Jul 1, 2024
@utkarsharma2 utkarsharma2 added this to the Airflow 2.9.3 milestone Jul 1, 2024
utkarsharma2 pushed a commit that referenced this pull request Jul 2, 2024
* Add `[webserver]update_fab_perms` to deprecated configs

`[webserver]update_fab_perms` is deprecated in favour of `[fab]update_fab_perms` and has been a breaking change since 2.9.0. This PR adds the config to the deprecated config list to properly inform users and have both options work at the moment

* Move provider config loading into init_appbuilder

* Remove deprecated webserver config options from config.yml

* Remove fallbacks

* Remove update_fab_perms and return the provider initialization

* Include FAB config in core configuration reference

* Update docs/apache-airflow/faq.rst

* Fix doc build error

(cherry picked from commit e24b7c1)
utkarsharma2 pushed a commit that referenced this pull request Jul 2, 2024
* Add `[webserver]update_fab_perms` to deprecated configs

`[webserver]update_fab_perms` is deprecated in favour of `[fab]update_fab_perms` and has been a breaking change since 2.9.0. This PR adds the config to the deprecated config list to properly inform users and have both options work at the moment

* Move provider config loading into init_appbuilder

* Remove deprecated webserver config options from config.yml

* Remove fallbacks

* Remove update_fab_perms and return the provider initialization

* Include FAB config in core configuration reference

* Update docs/apache-airflow/faq.rst

* Fix doc build error

(cherry picked from commit e24b7c1)
ephraimbuddy added a commit that referenced this pull request Jul 2, 2024
* Add `[webserver]update_fab_perms` to deprecated configs

`[webserver]update_fab_perms` is deprecated in favour of `[fab]update_fab_perms` and has been a breaking change since 2.9.0. This PR adds the config to the deprecated config list to properly inform users and have both options work at the moment

* Move provider config loading into init_appbuilder

* Remove deprecated webserver config options from config.yml

* Remove fallbacks

* Remove update_fab_perms and return the provider initialization

* Include FAB config in core configuration reference

* Update docs/apache-airflow/faq.rst

* Fix doc build error

(cherry picked from commit e24b7c1)
@DartVeDroid
Copy link

@ephraimbuddy,
I'm getting FutureWarning: section/key [webserver/update_fab_perms] has been deprecated, you should use[fab/update_fab_perms] instead., despite having already set that in airflow.cfg.
Is that supposed to happen?
Also, why no spacebar after "use"? =D

@ephraimbuddy
Copy link
Contributor Author

@ephraimbuddy, I'm getting FutureWarning: section/key [webserver/update_fab_perms] has been deprecated, you should use[fab/update_fab_perms] instead., despite having already set that in airflow.cfg. Is that supposed to happen? Also, why no spacebar after "use"? =D

Can you check if you have webserver/update_fab_perms set somewhere? Feel free to contribute a fix for the space issue you discovered

romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
* Add `[webserver]update_fab_perms` to deprecated configs

`[webserver]update_fab_perms` is deprecated in favour of `[fab]update_fab_perms` and has been a breaking change since 2.9.0. This PR adds the config to the deprecated config list to properly inform users and have both options work at the moment

* Move provider config loading into init_appbuilder

* Remove deprecated webserver config options from config.yml

* Remove fallbacks

* Remove update_fab_perms and return the provider initialization

* Include FAB config in core configuration reference

* Update docs/apache-airflow/faq.rst

* Fix doc build error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants