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 type of sshexecutor stdin parameter #314

Merged
merged 5 commits into from
Nov 10, 2023

Conversation

giffels
Copy link
Member

@giffels giffels commented Nov 9, 2023

During migration of our active C/T instances to a different host, we are using the first time the HTCondorSiteAdapter in combination with the SSHExecutor. In addition, the HTCondorSiteAdapter is the only adapter that uses the stdin to add input to the condor_submit.

That triggered a bug in the SSHExecutor, which assumed that asyncssh requires stdin to be in bytes format. However, according to the documentation, it is required to be in string format.

This pull request changes the type of SSHExecutor from byte format to string format.

@giffels giffels added the bug Something isn't working label Nov 9, 2023
@giffels giffels self-assigned this Nov 9, 2023
@giffels giffels changed the title Fix type of sshexecutor stdin variable Fix type of sshexecutor stdin parameter Nov 9, 2023
@giffels giffels mentioned this pull request Nov 9, 2023
6 tasks
@giffels giffels requested review from a team, rfvc, mschnepf and maxfischer2781 and removed request for a team November 9, 2023 11:32
python3 -m pip install --upgrade pip
python3 -m pip install .[contrib]
python3 -m pip install coverage codecov
python3.11 -m pip install --upgrade pip
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated change to fix the Mac OS deployment test, which by default uses Python 3.12 now. However, some of our dependencies are not yet supporting 3.12 by now.

Copy link
Contributor

@rfvc rfvc left a comment

Choose a reason for hiding this comment

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

Just one comment

tests/utilities_t/executors_t/test_sshexecutor.py Outdated Show resolved Hide resolved
@giffels giffels requested a review from rfvc November 9, 2023 18:28
@codecov-commenter
Copy link

codecov-commenter commented Nov 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (36ac7c1) 98.86% compared to head (5bc20d4) 98.86%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #314   +/-   ##
=======================================
  Coverage   98.86%   98.86%           
=======================================
  Files          55       55           
  Lines        2210     2210           
=======================================
  Hits         2185     2185           
  Misses         25       25           
Files Coverage Δ
tardis/utilities/executors/sshexecutor.py 100.00% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@maxfischer2781 maxfischer2781 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the changes.

Copy link
Member

@mschnepf mschnepf left a comment

Choose a reason for hiding this comment

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

It looks good to me. 👍 Thanks

@giffels giffels added this pull request to the merge queue Nov 10, 2023
Merged via the queue into MatterMiners:master with commit 980d3d3 Nov 10, 2023
17 checks passed
@giffels giffels deleted the fix/ssh-executor-stdin branch November 10, 2023 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants