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

Add skip_on_exit_code to SSHOperator #36303

Merged
merged 1 commit into from
Dec 21, 2023
Merged

Add skip_on_exit_code to SSHOperator #36303

merged 1 commit into from
Dec 21, 2023

Conversation

dolfinus
Copy link
Contributor

@dolfinus dolfinus commented Dec 19, 2023

Now SSHOperator supports option skip_on_exit_code, just like BashOperator, PythonVirtualenvOperator and others.


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

@boring-cyborg boring-cyborg bot added area:core-operators Operators, Sensors and hooks within Core Airflow area:providers kind:documentation provider:ssh labels Dec 19, 2023
@dolfinus dolfinus force-pushed the main branch 3 times, most recently from 1781763 to d95e1c1 Compare December 19, 2023 11:15
@dolfinus dolfinus marked this pull request as ready for review December 19, 2023 11:31
@dolfinus dolfinus requested a review from potiuk as a code owner December 19, 2023 11:31
@dolfinus dolfinus force-pushed the main branch 2 times, most recently from 0059e4a to c728511 Compare December 19, 2023 16:14
@Taragolis Taragolis removed the area:core-operators Operators, Sensors and hooks within Core Airflow label Dec 19, 2023
Comment on lines +231 to +237
if expected_exc is None:
operator.execute({})
else:
with pytest.raises(expected_exc):
operator.execute({})
Copy link
Contributor

@josh-fell josh-fell Dec 19, 2023

Choose a reason for hiding this comment

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

Suggested change
if expected_exc is None:
operator.execute({})
else:
with pytest.raises(expected_exc):
operator.execute({})
with pytest.raises(expected_exc):
operator.execute({})

It's a nit really, but you don't need the branch here.

Copy link
Contributor Author

@dolfinus dolfinus Dec 20, 2023

Choose a reason for hiding this comment

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

There are test cases with exit code 0 which does not raise exception. But passing None or empty tuple to pytest.raises is not supported:
ValueError: Expected an exception type or a tuple of exception types, but got None. Raising exceptions is already understood as failing the test, so you don't need any special code to say 'this should never raise an exception'.
ValueError: Expected an exception type or a tuple of exception types, but got (). Raising exceptions is already understood as failing the test, so you don't need any special code to say 'this should never raise an exception'.

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.

Nit

airflow/providers/ssh/operators/ssh.py Outdated Show resolved Hide resolved
@potiuk potiuk merged commit 73cd0d5 into apache:main Dec 21, 2023
52 checks passed
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.

6 participants