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

Fix message raised on excepion in SFTP deferrable operator #38525

Closed
wants to merge 1 commit into from

Conversation

Bowrna
Copy link
Contributor

@Bowrna Bowrna commented Mar 27, 2024

related: #38270
fix PT012 checks for sftp.


^ 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.

@Taragolis
Copy link
Contributor

@Bowrna Bowrna force-pushed the pt012-rule-sftp branch 2 times, most recently from 08fa973 to b51ccca Compare March 28, 2024 05:46
@Bowrna
Copy link
Contributor Author

Bowrna commented Apr 2, 2024

hello @Taragolis I fixed the conflicts. could you check this PR and let me know your views? Thanks

@potiuk potiuk changed the title pt012 rule enabled for sftp Fix message raised on excepion in SFTP deferrable operator Apr 2, 2024
@potiuk
Copy link
Member

potiuk commented Apr 2, 2024

Ah I see we also have #38518 with the same changes @shahar1 - do you want to proceed with yours?

@Bowrna
Copy link
Contributor Author

Bowrna commented Apr 3, 2024

Ah I see we also have #38518 with the same changes @shahar1 - do you want to proceed with yours?

I didn't know @shahar1 is working on this changes, my bad! :)

@shahar1
Copy link
Contributor

shahar1 commented Apr 3, 2024

Ah I see we also have #38518 with the same changes @shahar1 - do you want to proceed with yours?

I prefer so :)

@Bowrna no worries, feel free to review my PR and add suggestions!

@potiuk
Copy link
Member

potiuk commented Apr 3, 2024

Ah I see we also have #38518 with the same changes @shahar1 - do you want to proceed with yours?

I prefer so :)

@Bowrna no worries, feel free to review my PR and add suggestions!

Suggestion @shahar1 . Look at this code, review and merge it and make @Bowrna co-author -> https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors : never hurts.

@shahar1
Copy link
Contributor

shahar1 commented Apr 3, 2024

Ah I see we also have #38518 with the same changes @shahar1 - do you want to proceed with yours?

I prefer so :)
@Bowrna no worries, feel free to review my PR and add suggestions!

Suggestion @shahar1 . Look at this code, review and merge it and make @Bowrna co-author -> https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors : never hurts.

Great suggestion! I will remember this option for similar occasions :)
I merged changes from this PR and added @Bowrna as a co-author to mine.

@potiuk
Copy link
Member

potiuk commented Apr 3, 2024

Needs rebase and solving conflict though :)

@shahar1
Copy link
Contributor

shahar1 commented Apr 3, 2024

Needs rebase and solving conflict though :)

Oh woops, I did it in mine: #38518
Sorry for the confusion

@potiuk
Copy link
Member

potiuk commented Apr 3, 2024

🤦

@potiuk
Copy link
Member

potiuk commented Apr 3, 2024

Fixed in #38518

@potiuk potiuk closed this Apr 3, 2024
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.

4 participants