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

Move SMTP Provider to new structure #46067

Closed

Conversation

shubhamraj-git
Copy link
Contributor

@shubhamraj-git shubhamraj-git commented Jan 26, 2025

related to #46045

  • No pending PR's for SMTP

We have a DRAFT PR opened yesterday which affects SMTP Provider by @hussein-awala

  • Ran breeze testing providers-tests --test-type 'Providers[ssh]'
  • Ran Static checks

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@shubhamraj-git
Copy link
Contributor Author

@hussein-awala I see we have DRAFT PR which relates to SMTP Provider, this migration can create conflicts. Are you okay with it or should we postpone this migration till you merge your PR?
This PR is ready to be merge state.

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the process of moving a provider is almost automated (according to some comments from Jarek in other PRs if I'm not mistaken), I prefer to wait until the other PR is merged.

@shubhamraj-git
Copy link
Contributor Author

Yes, It's an automated process. I can do it post your PR gets merged.

@potiuk
Copy link
Member

potiuk commented Jan 26, 2025

Yeah. It looks like we could start to batch those changes - since they are generating conflicts.

@potiuk
Copy link
Member

potiuk commented Jan 27, 2025

As the process of moving a provider is almost automated (according to some comments from Jarek in other PRs if I'm not mistaken), I prefer to wait until the other PR is merged.

Maybe we can merge it anyway. It will be rather easy to rebase your PR @hussein-awala and we still need to agree on the change from your PR :)

@o-nikolas
Copy link
Contributor

As the process of moving a provider is almost automated (according to some comments from Jarek in other PRs if I'm not mistaken), I prefer to wait until the other PR is merged.

Maybe we can merge it anyway. It will be rather easy to rebase your PR @hussein-awala and we still need to agree on the change from your PR :)

Given that the email discussions are still ongoing and the PR remains in draft. Shall we merge this one?

Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything holding us back? I think this looks good to merge.

@hussein-awala
Copy link
Member

I split my PR into many smaller ones, I need to merge #46219 before rebasing and merging this one. I'll try to fix the failing tests tomorrow or Saturday.

@potiuk
Copy link
Member

potiuk commented Feb 7, 2025

Instead of rebasing it I created #46556 anew - it seems that it should work out of the box - easier than solve conflicts with this one.

@potiuk potiuk closed this Feb 7, 2025
@shubhamraj-git
Copy link
Contributor Author

@potiuk Yaa, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants