Taragolis commented on code in PR #23560: URL: https://github.com/apache/airflow/pull/23560#discussion_r901133246
########## docs/apache-airflow/security/secrets/secrets-backend/index.rst: ########## @@ -101,8 +143,8 @@ Roll your own secrets backend A secrets backend is a subclass of :py:class:`airflow.secrets.BaseSecretsBackend` and must implement either :py:meth:`~airflow.secrets.BaseSecretsBackend.get_connection` or :py:meth:`~airflow.secrets.BaseSecretsBackend.get_conn_value`. -After writing your backend class, provide the fully qualified class name in the ``backend`` key in the ``[secrets]`` -section of ``airflow.cfg``. +After writing your backend class, provide the fully qualified class name in the ``backend`` key or provide Review Comment: Nothing personal but let me explain how it looks like for my side. I invest some my time to check how it possible to do it right now with same approach and found that withouts hacks in runtime it is hardly possible but you've told me: > So maybe this means you can't copy the existing "upgrade" examples exactly -- maybe you have to do something a little different, a little more difficult Which more like for me `I don't know how to do it, but think better and do it better` - that is last thing that I or someone else expects see on review (both on open source or commercial development) . If you have suggestion how to do it better right now without rewrite a lot of stuff - feel free to suggest. --- Right now Secrets Backends ignore "better approach", implicit upgrades values and do not track it. I think everyone sleep well https://github.com/apache/airflow/blob/2936759256273a66cf6343e451d81013a95ec47e/airflow/configuration.py#L1506-L1510 --- Just for reduce your or someone else time for investigate how to do it "in right way right now". ### Specify directly AirflowConfigParser.deprecated_options You can't just placed in AirflowConfigParser.deprecated_options something like `('secrets', 'backends_config'): ('secrets', 'backend', '2.4.0')` because we need to check that **[secrets] backend** defined and upgrades required. ```python def _upgrade_secrets_backend(self): old_backend = self.get(section='secrets', key='backend', fallback='') if old_backend: ... ``` Code above always will return warning even if user do not define anything in **[secrets] backend** _DeprecationWarning: The backend option in [secrets] has been renamed to backends_config - the old setting has been used, but please update your config._ _Possible solution?_ Define in deprecation config `('secrets', 'backends_config'): ('secrets', 'backend', '2.4.0')` right after upgrade. Which might be unclear for developers, because they should keep it in their mind that something outside could change AirflowConfigParser.deprecated_options ### Deprecate [secrets] backend_kwargs It is not possible right now by define something in AirflowConfigParser. _Possible solution?_ Show warning that this config doesn't use anymore and anywhere on upgrade. However right now we also could warn users with custom message on init secrets backend ### Warning show that only one option deprecated We need to inform user by this message ``` DeprecationWarning: The backend and backend_kwargs options in [secrets] has been moved to backends_config - the old setting has been used, but please update your config. ``` But now we can get only predefined ```python warnings.warn( f'The {deprecated_name} option in [{deprecated_section}] has been moved to the {key} option ' f'in [{section}] - the old setting has been used, but please update your config.', DeprecationWarning, stacklevel=3, ) ``` _Possible solution?_ Rewrite AirflowConfigParser.deprecated_options and methods which check and show this warning to make it more generic. --- I'm absolutely agree with your idea that we need to have only one approach with deprecated values. But right now Secrets Backend doesn't use this approach in anyway and without changes in AirflowConfigParser it is hardly possible. And this PR only about extends functional of Secrets Backend not changes in AirflowConfigParser. If we want to change AirflowConfigParser better to open separate PR in this case if we need to revert back changes we would revert "Add advanced secrets backend configurations" or "Make AirflowConfigParser more generic with deprecation warning" but not both. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
