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 support for FTPing XML to server #39

Merged
merged 1 commit into from Jul 24, 2018
Merged

Add support for FTPing XML to server #39

merged 1 commit into from Jul 24, 2018

Conversation

gravesm
Copy link

@gravesm gravesm commented Jul 23, 2018

This adds an alternative output option for the generated XML feed,
allowing it to be sent out over FTPS. It is in preparation for moving
this to an AWS Lambda.

The size of the Academic Analytics feed can get pretty big, and given
the constraints of Lambda, this uses a Unix pipe to buffer the XML. I'm
leaving the existing CLI in place because it makes it easier to test out
in development.

This also drops old versions of Python from the test suite. Although the
FTP server used for testing claims to support 2.6+, the TLS support
wasn't working in 2.7. Lambda uses 3.6 anyways.

This adds an alternative output option for the generated XML feed,
allowing it to be sent out over FTPS. It is in preparation for moving
this to an AWS Lambda.

The size of the Academic Analytics feed can get pretty big, and given
the constraints of Lambda, this uses a Unix pipe to buffer the XML. I'm
leaving the existing CLI in place because it makes it easier to test out
in development.

This also drops old versions of Python from the test suite. Although the
FTP server used for testing claims to support 2.6+, the TLS support
wasn't working in 2.7. Lambda uses 3.6 anyways.
@gravesm
Copy link
Author

gravesm commented Jul 23, 2018

Oh, also, yes, I am aware this commit includes a cert+key pair.

for article in articles():
f(article)
if ftp:
r, w = os.pipe()
Copy link
Member

@JPrevost JPrevost Jul 23, 2018

Choose a reason for hiding this comment

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

Just a comment and there may be very good reasons for not doing this, but it might be nice to move this setup code to the PipeWriter class to make this if as clean as the else for ease of reading.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Since it's reading all the CLI switches it's probably not clean to do that. Oh well.

Copy link
Member

@JPrevost JPrevost left a comment

Choose a reason for hiding this comment

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

This seems solid. I didn't actually run it though so my approval is based on you having confirmed this does what it needs to ;)

@gravesm gravesm merged commit 302942f into master Jul 24, 2018
@gravesm gravesm deleted the ftp branch July 24, 2018 20:18
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