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

Added paramikos's callbacks for sftp get and put operations based on:… #2170

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

martinwittmann
Copy link

Adding support for paramiko's callbacks for paramiko's put + get operations, which is based on #2044

The changes are trivial and could end several github issues and pull requests.

Here's a quick list:

@martinwittmann
Copy link
Author

Looks like the builds are failing randomly (e.g. https://travis-ci.org/github/fabric/fabric/jobs/774254250).
Could somebody be so kind and give me a hint what to do about the failing builds?

@davidjmemmett
Copy link
Contributor

Hi @martinwittmann,

Thanks for the PR!
The Travis CI is due to be replaced with Circle CI soon, so please don't worry about the failed builds. I've checked out your branch and confirmed that the tests pass as-is.

A couple of issues that may hinder these changes being merged in:

  • Tests that prove existing functionality isn't broken
  • Tests to support the code you've added
  • An entry in the changelog describing the change

I'd suggest adding at least one new test which passes a callback in, and verifies that it is called.

Cheers,
David

@martinwittmann
Copy link
Author

I added a test + updated the changelog.
But I can't get any tests to run locally. I used https://docs.fabfile.org/en/1.12.1/running_tests.html to set everything up, but when running

nosetest tests/

I get "Ran 0 tests in 0.000s", and running fab test complains that it can't find a collection named fabfile.

It seems like I'm missing something.
Thanks for any pointers that might help get those test running!

@davidjmemmett
Copy link
Contributor

davidjmemmett commented Jun 24, 2021 via email

@martinwittmann
Copy link
Author

Thanks a lot David for helping me out!
I added 2 integration tests for get and put operations verifying that the callbacks are actually being called.

If there is anything else that needs to be done to get this merged please let me know.

@martinwittmann
Copy link
Author

@davidjmemmett Is there anything I can do to get this merged?

@davidjmemmett
Copy link
Contributor

One for @bitprophet - I don't have merging permissions

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

Successfully merging this pull request may close these issues.

None yet

3 participants