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

Better Pipeline.remote_name handling #267

Merged
merged 4 commits into from
Nov 29, 2017
Merged

Conversation

sevein
Copy link
Member

@sevein sevein commented Nov 19, 2017

Read the commits for more details.

This pull request mitigates #266 (related to #236).

@sevein sevein added this to the 0.11.0 milestone Nov 19, 2017
@sevein sevein self-assigned this Nov 19, 2017
@scollazo
Copy link
Member

I think this will fail if the ss uses ssl with self-signed certificates.

@sevein
Copy link
Member Author

sevein commented Nov 21, 2017

I think this will fail if the ss uses ssl with self-signed certificates.

Ok, thank you @scollazo. I'll add a INSECURE_SKIP_VERIFY toggle.

@sevein
Copy link
Member Author

sevein commented Nov 22, 2017

@scollazo see c8c5b5f.

Copy link
Contributor

@ross-spencer ross-spencer left a comment

Choose a reason for hiding this comment

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

I can't see anything too major, suggestions:

  • parse_url() should be changed to parse_and_fix_url(): Rationale: the function name is confusing vs. other urlparse()-like functions out there (we're not just parsing). We're adding something new, e.g. making sure http is explicitly added if the scheme is empty.

  • url = parse_url(...), url = uri._replace(...) any instances where you're pairing these two operations (I count three?) I would change this into a single helper function to do both operations at the same time, turning the two lines into two. This is a personal preference whenever I see the same variable being manipulated like this within the same number of lines, so it wraps it in a way that looks more deliberate. It may also help unit testing.

  • url as a variable name could be improved (maybe from golang experience - url is just a difficult variable name.

  • Lines 104 and 344 in helpers.py the function call is constructed using a parameter:

    verify=not

  • not is a keyword in Python and unless this library explicitly requests the use of the word not, over (?) then i would suggest changing that to another value. (I can't remember not being used like this - so could just be me)

@qubot qubot force-pushed the dev/pipeline-remote-name-fixes branch 2 times, most recently from 80ff178 to ad618c0 Compare November 29, 2017 22:50
@sevein
Copy link
Member Author

sevein commented Nov 29, 2017

@ross-spencer, I've addressed your comments.

I've used vcrpy to test the new methods. It's a handy testing tool that Holly introduced a while ago where you can go beyond unit tests by recording the HTTP requests and responses and keep in them in the repo as fixtures.

Copy link
Contributor

@ross-spencer ross-spencer 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 @sevein thx for the additional changes.

Convert the value of remote_name into a ParsedResult because it's easier to
manipulate URLs with it. Default to http scheme when undefined.

This is related to #266.
This will allow the Dashboard to pass a public URL instead of having the SS
guess it using the REMOTE_ADDR header which is not reliable.
Clean up the code a bit, mainly `api/sword/helpers.py` which was not using the
Pipeline model to interact with the API. Other minor changes have been added.
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.

4 participants