Skip to content

CHC SFTP Downloads #352

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

Merged
merged 10 commits into from
Oct 28, 2020
Merged

CHC SFTP Downloads #352

merged 10 commits into from
Oct 28, 2020

Conversation

rumackaaron
Copy link
Contributor

Download CHC files through SFTP to allow for automation of CHC signal

@rumackaaron rumackaaron added the API addition New signals label Oct 21, 2020
@rumackaaron
Copy link
Contributor Author

@krivard @chinandrew The script now downloads files from the CHC ftp server and generates the CHC signal from those files. I'm not sure what we want to do about testing since I don't want to push the credentials to this repo.

@krivard
Copy link
Contributor

krivard commented Oct 22, 2020

You are right not to upload them to the repo in plaintext; we'll add them to the Ansible vault instead. @korlaxxalrok can get you set up.

client.set_missing_host_key_policy(AllowAnythingPolicy())

client.connect(ftp_conn["host"], username=ftp_conn["user"],
password=ftp_conn["pass"][1:] + ftp_conn["pass"][0],
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 1:?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is my super hi-tech encryption method. I store the password in plaintext as "dpasswor" and then the client connects using "password." It's almost as foolproof as RSA, just don't tell anyone!

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to do this if credentials are encrypted at rest in ansible?

@@ -25,6 +26,10 @@ def run_module():

logging.basicConfig(level=logging.DEBUG)

## download recent files from FTP server
logging.info("downloading recent files through SFTP")
download(params["cache_dir"], params["ftp_conn"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be mocked out for testing?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@krivard krivard requested a review from chinandrew October 22, 2020 19:15
@chinandrew
Copy link
Contributor

This looks good, just the open question on testing and the password obfuscation

# download!
for infile, outfile in filepaths_to_download.items():
callback_for_filename = functools.partial(print_callback, infile)
sftp.get(infile, outfile, callback=callback_for_filename)
Copy link
Contributor

Choose a reason for hiding this comment

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

callback is cool, didn't know of this feature in paramiko

@rumackaaron
Copy link
Contributor Author

Sorry for the delay in creating the tests, I've never worked with mocks before. Let me know what else you'd like me to do

@chinandrew
Copy link
Contributor

I'm getting the following test errors

ERROR test_load_data.py - KeyError: 'input_covid_file'
ERROR test_sensor.py - KeyError: 'input_covid_file'
ERROR test_update_sensor.py - KeyError: 'input_covid_file'

@rumackaaron
Copy link
Contributor Author

Can you copy the current params.json.template to params.json and see if the errors go away?

@chinandrew
Copy link
Contributor

Can you copy the current params.json.template to params.json and see if the errors go away?

Thanks, that did the trick. Any reason we can't just keep it in the params.json (the one in the tests folder)?

@rumackaaron
Copy link
Contributor Author

Can you copy the current params.json.template to params.json and see if the errors go away?

Thanks, that did the trick. Any reason we can't just keep it in the params.json (the one in the tests folder)?

Theoretically no, but I assume there's some reason that params.json is included in .gitignore

@capnrefsmmat
Copy link
Contributor

Mostly to prevent production params from getting into the repo. I think test params are fine to force-commit.

@chinandrew
Copy link
Contributor

So I had an old params.json present which I think was causing the issue. Normal behavior will just have it read the template if a params isn't present, which I think is fine. Just fix linting and should be good to go.

Copy link
Contributor

@chinandrew chinandrew left a comment

Choose a reason for hiding this comment

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

Linting still raising some issues on things like line length, but I think they snuck in the previous PR and are not due to this one, so I'm fine having those fixed in a separate PR. Tests are passing.

@krivard krivard merged commit d947887 into main Oct 28, 2020
@krivard krivard deleted the changehc branch February 9, 2021 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API addition New signals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants