-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
move connectionless email depreciation warning #31216
Conversation
warning was located inside the wrong block. now it will warn if there is no conn_id assigned
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:/apache/airflow/blob/main/CONTRIBUTING.rst)
|
except AirflowException: | ||
pass | ||
if smtp_user is None or smtp_password is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am taking a look at the issue trail and wondering if we only need an else block here with keeping all the existing code as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d argue the smtp_... = airflow_conn...
lines should be moved to the else block as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am taking a look at the issue trail and wondering if we only need an else block here with keeping all the existing code as it is.
I dont see how we would do that and be certain that the smtp creds came from the connection and not from the conf file without rework
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Holding till review from @hussein-awala as email/smtp is under refactor.
@hussein-awala can you please review?
if smtp_user is None or smtp_password is None: | ||
log.debug("No user/password found for SMTP, so logging in with no authentication.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why move this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because its a valid warning that no creds were supplied in the connection and will be using non authentication smtp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I mean does it not do its job at its original location? Why not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want it to trigger after ether a connection has been found or failback to airflow.cfg variables have been parsed
So i moved it after the above had been completed
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
depreciation warning was located inside the wrong block.
now it will warn if there is no conn_id assigned
closes: #19805