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.

Sign up for GitHub

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

Jump to bottom

Improve support for Windows #160

Closed
wants to merge 7 commits into from
Closed

Improve support for Windows #160

wants to merge 7 commits into from

Conversation

MichaelAquilina
Copy link
Owner

Fix for issues #155 and #156

Added support to ignore SSL errors if the environment variable PYTHONWARNINGS
is "ignore:Unverified HTTPS request"

@ranaur I've opened the pull request for you

Fix for issues #155 and #156
8b28cb2 
Added support to ignore SSL errors if the environment variable PYTHONWARNINGS
is "ignore:Unverified HTTPS request"
@MichaelAquilina MichaelAquilina mentioned this pull request Oct 28, 2018
Open
@MichaelAquilina
Copy link
Owner Author

It looks like the version of your code is outdated compared to master. Please rebase your code :)

Let me know if you need help with that.

MichaelAquilina
MichaelAquilina commented Oct 28, 2018
View reviewed changes
s4/clients/s3.py Outdated
if python_ignore == 'ignore:Unverified HTTPS request':
verifySSL = False
else:
verifySSL = True
Copy link
Owner Author

Choose a reason for hiding this comment

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

could you explain why you would want this?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Behind my corporate firewall I can't connect using HTTPS because the corporate proxy exchange the encryptation keys to inspect the traffic (Security Here is High).

Oh wow that's pretty horrible :| how do you connect to any website then? Most websites use HSTS nowadays which means they wont allow you to connect to them without an https connection.

MichaelAquilina
MichaelAquilina commented Oct 28, 2018
View reviewed changes
s4/commands/sync_command.py Outdated
if hasattr(sync_object, 'total_size'):
total_size = sync_object.total_size
else:
total_size = 0
Copy link
Owner Author

Choose a reason for hiding this comment

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

what is this for?

Copy link
Owner Author

Choose a reason for hiding this comment

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

The sync_object came None and it raised an exception. The code just assures the object really has the member before calling.

In that case what about:

total_size = sync_object.total_size if sync_object else 0

@ranaur
Copy link

ranaur commented Oct 29, 2018 via email

@ranaur
Copy link

ranaur commented Oct 29, 2018 via email

@ranaur
Copy link

ranaur commented Oct 29, 2018 via email

@ranaur
Copy link

ranaur commented Oct 29, 2018 via email

@MichaelAquilina
Copy link
Owner Author

Maybe the better alternative is to allow the user to specify additional certificates to use then. Does your company recommend against adding the certificate they provide to your system's certificate folder? A quick search on Google gave me this stack overflow post explaining how certificates are stored on windows: https://superuser.com/questions/411909/where-is-the-certificate-folder-in-windows-7#411930

@ranaur
Copy link

ranaur commented Oct 29, 2018 via email

@MichaelAquilina
Copy link
Owner Author

To me there are two separate problems to solve here:

  • Supporting Windows
  • Supporting machines that need to work with a proxy certificate

Let's focus on supporting windows first and then the proxy :)

The PR merge is not completely successful because cli.py does not exist anymore on master.

The way to run code directly is now to run using poetry (which is the package manager for this project).

pip install poetry
poetry run s4 sync

I was meant to update the contributing section explaining how to use poetry but forgot to - so my apologies about that.

Fix some display issues when using windows. To do this, it was create…
484c860 
…s a geturi_local method that display the URI in the local format (local means using \ in Windows machine).

Now both clients internally uses always the posix format. local.py translates to local (windows) path format when needs to go to the filesystem.

Fix to allow honoring the environment variable REQUESTS_CA_BUNDLE to search for the certification files for the SSL connection. This is the standard variable to use. For some reason BOTO3 doesn't honor it, but urllib3 do, so many other connections APIs.
@ranaur
Copy link

ranaur commented Oct 30, 2018 via email

@ranaur
Copy link

ranaur commented Oct 30, 2018 via email

@MichaelAquilina
Copy link
Owner Author

we need to manually install python-magic

Could you add the installation instructions for this to the README?

I hope it is finally ok, and I can try to do the daemon as soon as I get some time.

Lets keep that in a separate Pull Request to prevent making this one too large

I'll review your code now :)

MichaelAquilina
MichaelAquilina commented Oct 30, 2018
View reviewed changes
TODO Outdated
utf-8 filename

Support validation of a PEM file to validate SSL/no-validate option
Put on config file?
Copy link
Owner Author

Choose a reason for hiding this comment

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

could you remove this file from the PR?

MichaelAquilina
MichaelAquilina commented Oct 30, 2018
View reviewed changes
cli.py Outdated
@@ -0,0 +1,152 @@
# -*- coding: utf-8 -*-

import argparse
Copy link
Owner Author

Choose a reason for hiding this comment

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

this file shouldnt be here - please remove it

MichaelAquilina
MichaelAquilina commented Oct 30, 2018
View reviewed changes
Copy link
Owner Author

@MichaelAquilina MichaelAquilina left a comment

Choose a reason for hiding this comment

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

There are quite a few issues with this existing PR which makes it hard to review correctly.

What I would recommend is that we start working on each "logical" change one by one - each its own Pull Request. That makes it easier to focus on getting each change correct and merged in as early as possible for you.

To me we should have a separate PR for:

  1. getting path working correctly on Windows
  2. Adding support for alternative SSL options

There are a lot of tutorials online about how to open Pull Requests on github that you could follow. You can start with this one for example: https://help.github.com/articles/creating-a-pull-request-from-a-fork/

In terms of getting paths to work. Could you give me an example of a path you are inserting into windows so that I could test it out? I think there is some lack of clarity about what should be entered which I can work on making clearer through some minor changes to the text.

@MichaelAquilina
Copy link
Owner Author

To start you off I would run the following commands:

git checkout master
git pull upstream master
git checkout -b windows_path_fixes

And then start adding your changes :)

@ranaur
Copy link

ranaur commented Oct 30, 2018 via email

@ranaur
Copy link

ranaur commented Oct 30, 2018 via email

@ranaur
Copy link

ranaur commented Oct 30, 2018 via email

@ranaur
Copy link

ranaur commented Oct 30, 2018 via email

@MichaelAquilina
Copy link
Owner Author

There are definitely some complications around making this work well on both s3 (which uses \ for separators) and windows (which uses /). I think due to its complexity it make make more sense to leave me to try fix it and I'll add you as a review to test it when that's done if that's ok.

With regards SSL, I could open a PR for you but its much easier if you learn to open PRs yourself :)

git checkout master
git pull upstream master
git checkout -b ssl_options
# commit your changes

Then go to github.com/ranaur/s4 and github should prompt you to open a new pull request here

@MichaelAquilina
Copy link
Owner Author

Closing this PR in favour of #165 as agreed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MichaelAquilina @ranaur