Skip to content

log handler deprecated filename_template argument removal - #41552

Merged
uranusjr merged 4 commits into
apache:mainfrom
dirrao:log_handler_deprecated_filename_template_removal
Aug 20, 2024
Merged

log handler deprecated filename_template argument removal#41552
uranusjr merged 4 commits into
apache:mainfrom
dirrao:log_handler_deprecated_filename_template_removal

Conversation

@dirrao

@dirraodirrao commented Aug 17, 2024

Copy link
Copy Markdown
Contributor

Passing filename_template to a log handler is deprecated and has no effect. So, removing deprecated filename_template argument in log handler.

@dirraodirrao self-assigned this Aug 17, 2024
@dirraodirrao added the airflow3.0:candidate Potential candidates for Airflow 3.0 label Aug 17, 2024
@dirrao
dirrao requested review from potiuk and uranusjrAugust 17, 2024 11:05
@uranusjruranusjr added the airflow3.0:breaking Candidates for Airflow 3.0 that contain breaking changes label Aug 17, 2024
Comment threadairflow/providers/microsoft/azure/log/wasb_task_handler.py Outdated
@uranusjr
uranusjr merged commit 716c430 into apache:mainAug 20, 2024
@eladkal

Copy link
Copy Markdown
Contributor

This PR involves both core and provider changes.
Please clarify what it means for new versions of providers (that will include this PR) but installed with Airflow 2.
Something is odd here.

The parameter in question is public API which is why we deprecate it first. Now main is Airflow 3 so we can remove it but providers must still be compatible with Airflow 2... so how can we remove it from providers safely now?

@dirrao

Copy link
Copy Markdown
ContributorAuthor

This PR involves both core and provider changes. Please clarify what it means for new versions of providers (that will include this PR) but installed with Airflow 2. Something is odd here.

Right now, this field is not being used and has no effect. The filename_template is optional argument with default value None.
We can still able to install and use latest providers on airflow 2 without any issue.

The parameter in question is public API which is why we deprecate it first. Now main is Airflow 3 so we can remove it but providers must still be compatible with Airflow 2... so how can we remove it from providers safely now?

As I said in the above statement, the providers can still work with airflow 2.

@potiuk

potiuk commented Aug 20, 2024

Copy link
Copy Markdown
Member

@eladkal is right - if someone installs the new provider witn an old configuration in Airlfow 2 with filename_template, it will just fail with "attemptign to iniitalize handlew with "filename_template" argument passed but it's missing" (or smth like that).

@dirrao

dirrao commented Aug 20, 2024

Copy link
Copy Markdown
ContributorAuthor

I see the keyword arguments for the all the

@eladkal is right - if someone installs the new provider witn an old configuration in Airlfow 2 with filename_template, it will just fail with "attemptign to iniitalize handlew with "filename_template" argument passed but it's missing" (or smth like that).

Yah. You are right. Most of them are supporting keyword arguments except one/two (like. es handler).

@dirrao

Copy link
Copy Markdown
ContributorAuthor

@eladkal / @potiuk
Let me know if you want me to revert it for providers alone.

@potiuk

Copy link
Copy Markdown
Member

Just reverting will not be enough, but I think if the base Handler will accept extra arguments (and ignores them) we should be home

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

airflow3.0:breakingCandidates for Airflow 3.0 that contain breaking changesairflow3.0:candidatePotential candidates for Airflow 3.0area:loggingarea:providersprovider:alibabaprovider:amazonAWS/Amazon - related issuesprovider:apache-hdfsprovider:elasticsearchprovider:googleGoogle (including GCP) related issuesprovider:microsoft-azureAzure-related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@dirrao@eladkal@potiuk@uranusjr@romsharon98@kaxil