Skip to content

Conversation

@kyungjunleeme
Copy link
Contributor

@kyungjunleeme kyungjunleeme commented Jul 2, 2025

Related to

This PR addresses https://github.com/apache/airflow/issues/3190971584](https://github.com/apache/airflow/pull/52618) by adding missing unit tests for the dry_run() method in SFTPToWasbOperator.

What this PR adds

  • test_dry_run_logs_and_skips_real_action
    Verifies that in Airflow 3+:

    • dry_run() logs the “Process will upload…” and “Executing delete…” messages
    • No actual load_file or delete_file calls are made
  • test_dry_run_raises_not_implemented
    Mocks AIRFLOW_V_3_0_PLUS = False to simulate Airflow < 3.0, and asserts that dry_run() raises NotImplementedError with the message:


^ 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 airflow-core/newsfragments.

@kyungjunleeme kyungjunleeme marked this pull request as draft July 2, 2025 07:34
@kyungjunleeme
Copy link
Contributor Author

kyungjunleeme commented Jul 2, 2025

image At first, I wasn’t aware there was a rule regarding the use of caplog, so I thought it absolutely had to be changed. But now I'm curious to hear the opinion of the Airflow maintainers on this.

def dry_run(self) -> None:
if not AIRFLOW_V_3_0_PLUS:
raise NotImplementedError("Not implemented for Airflow 3.")
raise NotImplementedError("dry_run() is only supported in Airflow 3.0+.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this message is more clear than before. How about ur opinion?

@kyungjunleeme kyungjunleeme force-pushed the test/sftp-to-wasb-operator-coverage branch from f3a6d98 to f3431ac Compare July 2, 2025 07:57
@kyungjunleeme kyungjunleeme marked this pull request as ready for review July 2, 2025 07:57
@kyungjunleeme kyungjunleeme force-pushed the test/sftp-to-wasb-operator-coverage branch from f3431ac to 9c6c4b5 Compare July 3, 2025 00:44
@kyungjunleeme
Copy link
Contributor Author

@kaxil hello,

This issue is related to the previous PR(#52618 ) and the message section. I have added a test case to cover this scenario.
Could you please take a look when you have a moment?

@kyungjunleeme kyungjunleeme force-pushed the test/sftp-to-wasb-operator-coverage branch 4 times, most recently from d281a96 to aa0cc88 Compare July 4, 2025 07:25
@kyungjunleeme
Copy link
Contributor Author

kyungjunleeme commented Jul 4, 2025

@potiuk

I’m still getting used to how things work in open-source, so I wanted to ask:
Is it okay to mention someone to request a review if a PR hasn’t received any feedback yet?

The PR is related to a recently merged one, so I thought it might make sense to get input from someone involved in that.

and If no reviewer is assigned to a PR, is it okay to mention someone?
I'm wondering if there's an implicit rule around that.
When I open a PR and no reviewer gets assigned, I’m a bit worried it might get overlooked.


Could you review my PR ?

@potiuk
Copy link
Member

potiuk commented Jul 4, 2025

and If no reviewer is assigned to a PR, is it okay to mention someone?
I'm wondering if there's an implicit rule around that.
When I open a PR and no reviewer gets assigned, I’m a bit worried it might get overlooked.

Better to just "remind" but not mention anyone. This will put it on the top and someone who is actually looking might have another chance not to overlook it.

@potiuk
Copy link
Member

potiuk commented Jul 4, 2025

Unless someone already commented and you continue conversation of course.

@kyungjunleeme
Copy link
Contributor Author

I just wanted to check — when you say "just remind", do you mean adding a comment without tagging anyone, like "Just a gentle reminder in case this was overlooked"?

@potiuk
Copy link
Member

potiuk commented Jul 4, 2025

yes

@potiuk
Copy link
Member

potiuk commented Jul 4, 2025

Generally - it's up to you as author to lead your PR to completion. But you have to be mindful that people here are volunteers - so you neded to figure out what's best on your own, that's the beauty and difficulty of being in opesource that in many cases this is all about humans and empathy and communication and you will not get ready recipes what to do.

Eventually - use your own judgment what is best. And sometimes you will make mistakes and be corrected or reminded - but eventually - you are responsible to do what's good.

@kyungjunleeme
Copy link
Contributor Author

Yes, I'm learning a lot from various aspects while contributing to open source. Thank you for your thoughtful advice.

@kyungjunleeme kyungjunleeme force-pushed the test/sftp-to-wasb-operator-coverage branch 2 times, most recently from c1d0b05 to c5aad32 Compare July 10, 2025 14:28
@kyungjunleeme
Copy link
Contributor Author

kyungjunleeme commented Jul 11, 2025

@kaxil
If you have some time, I'd really appreciate it if you could review this PR.
I’d like to take full responsibility for finishing it properly, and your feedback would be invaluable to ensure everything is in good shape.

Thank you in advance for your time and support!

@kyungjunleeme kyungjunleeme force-pushed the test/sftp-to-wasb-operator-coverage branch from c5aad32 to 93ceb94 Compare July 11, 2025 08:29
@kyungjunleeme kyungjunleeme force-pushed the test/sftp-to-wasb-operator-coverage branch from 93ceb94 to 9ae346b Compare July 27, 2025 14:31
@github-actions
Copy link

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.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 11, 2025
@github-actions github-actions bot closed this Sep 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:microsoft-azure Azure-related issues stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants