Skip to content
This repository has been archived by the owner on Sep 30, 2020. It is now read-only.

Issue fix #42 & #43 #47

Merged
merged 3 commits into from
Jan 26, 2016
Merged

Issue fix #42 & #43 #47

merged 3 commits into from
Jan 26, 2016

Conversation

karolyi
Copy link
Contributor

@karolyi karolyi commented Jan 25, 2016

I am the wonderfully reorganized stack of commits from the chaos that emerged with a bad constellation under travis and cram:

aiiie/cram#7

Please merge me, so the world can be a better place.

In other news, I happen to fix 2 bugs at the same time: #42 and #43.

@karolyi karolyi changed the title Issue fix 42 43 Issue fix #42 & #43 Jan 25, 2016
@karolyi
Copy link
Contributor Author

karolyi commented Jan 25, 2016

@esc ?

@esc
Copy link
Contributor

esc commented Jan 25, 2016

Was in a meeting, sry.

- pyb install_dependencies

- travis_retry pip install -U pip
- travis_retry pip install git+https://github.com/karolyi/pybuilder.git cram coveralls
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think we need to install cram here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can you link me the issue in pybuilder that fixes the cram plugin?

@esc
Copy link
Contributor

esc commented Jan 25, 2016

I rebased on current master.

@karolyi
Copy link
Contributor Author

karolyi commented Jan 25, 2016

pybuilder/pybuilder#316

@esc
Copy link
Contributor

esc commented Jan 25, 2016

Sorry, ran into password reset troubles, will continue tomorrow.

@esc
Copy link
Contributor

esc commented Jan 25, 2016

I set myself a new password with an Umlaut and afp-cli was fine, but the afp-core on the other side didn't like it, I think. I am not sure though, because then I couldn't log into my local Linux machine anymore. Although that may have another reason.

I'd like to hold off merging this until it is confirmed to work, of which I am not yet convinced.

This was referenced Jan 25, 2016
@karolyi
Copy link
Contributor Author

karolyi commented Jan 26, 2016

rewritten, again.

only role name parameter can be passed with UTF-8 characters now, which is part of the url, so it gets encoded.

@karolyi
Copy link
Contributor Author

karolyi commented Jan 26, 2016

fixes issues #42 and #43

self.api_url = kwargs.get('api_url', None)
self.ssl_verify = kwargs.get('ssl_verify', True)

def call_api(self, url_suffix):
"""Send a request to the aws federation proxy"""
url_orig = '{0}{1}'.format(self.api_url, url_suffix)
# Workaround: request seems to mystically fail with utf8 stuff
url = requests.utils.requote_uri(url_orig)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this line now, or should we leave it in for good measure? If so, perhaps adapt the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, maybe remove the comment but I'd definitely keep the variable, so it won't have to be compiled at the parameter handover time.

actually there was more that was removed, I just run through the whole source code and removed the non relevant parts, so this is what remained.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we should definately keep url_orig.

@esc
Copy link
Contributor

esc commented Jan 26, 2016

It looks good, only a few minor nitpicks.

Also, we have had some problems with the unicode_literals in the past in the afp project, so we should be vigilant for errors that may be caused by that future import.

karolyi added a commit that referenced this pull request Jan 26, 2016
Issue fix #42 & #43

great work! (taps self on the back)
@karolyi karolyi merged commit ec7145b into master Jan 26, 2016
@esc esc deleted the issue-fix-42-43 branch January 26, 2016 17:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants