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

Decouple SMTP Provider settings from the Airflow Core #37703

Closed
3 tasks done
Taragolis opened this issue Feb 26, 2024 · 8 comments · Fixed by #37711
Closed
3 tasks done

Decouple SMTP Provider settings from the Airflow Core #37703

Taragolis opened this issue Feb 26, 2024 · 8 comments · Fixed by #37711
Labels
area:core good first issue kind:meta High-level information important to the community provider:smtp

Comments

@Taragolis
Copy link
Contributor

Taragolis commented Feb 26, 2024

Body

By mistake we add coupling between SMTP provider and core in #36226.
Discussion for the reference: #37701 (comment)

Provider Changes

  • Move configurations into the provider configurations and get this settings from there

Airflow Core Changes

  • Deprecate Remove SMTP_DEFAULT_TEMPLATED_SUBJECT and SMTP_DEFAULT_TEMPLATED_HTML_CONTENT_PATH into the airflow/settings.py
    - E.g. on access raise a deprecation warning with suggestion to upgrade to the new version of the SMTP Provider
    - E.g. on set from airflow_local_settings.py raise a deprecation warning with suggestion to upgrade to the new version of the SMTP Provider and use configurations instead

Committer

  • I acknowledge that I am a maintainer/committer of the Apache Airflow project.
@eladkal
Copy link
Contributor

eladkal commented Feb 26, 2024

It should be part of #30530
@hussein-awala started working on it a while ago in
#30531

@Taragolis
Copy link
Contributor Author

Taragolis commented Feb 26, 2024

Not really. This one about remove unnecessary settings options from the SMPT Provider, which add accidentally into the SmtpNotifier and Airflow 2.8.1 my bad it would be add into Airflow 2.9, so it should be easier to remove it from core.

@Taragolis
Copy link
Contributor Author

So there is no such attributes into the settings.py into the Airflow 2.8.1 and 2.8.2

So we might just fix it into the provider, and remove it from the core until we release 2.9

@vchiapaikeo
Copy link
Contributor

vchiapaikeo commented Feb 26, 2024

Oh nice! If we can just remove those configs, that would make things a lot easier. I was thinking of swapping them over to provider configuration w/ something like this:

#37711

Wdyt? Probably need to fix up a few tests but need to run right now so can look a little closer tonight.

@potiuk
Copy link
Member

potiuk commented Feb 26, 2024

We just discussed it in release management channel - we will just remove the settings altogether (and yes replace with config) - we will also yank 1.6.0 (likely) and release 1.7.0 and also switch constraints for Airfrlow 2.8.1 and 2.8.2 to use smtp 1.5.0

@potiuk
Copy link
Member

potiuk commented Feb 26, 2024

You can join #release-management channel on Slack for that one @vchiapaikeo :)

@vchiapaikeo
Copy link
Contributor

Awesome, sounds good. Sorry about all the hassle here. Will get to fixing up that PR tonight!

@potiuk
Copy link
Member

potiuk commented Feb 26, 2024

Well. We did not do a good job on preventing it during review, so don't put all the blame on you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core good first issue kind:meta High-level information important to the community provider:smtp
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants