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
Support developer authentication flow in bananas-cli #20
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really should be 4 commits. I don't really mind it is in a single PR, but I do mind it is in a single commit. That makes it really difficult for future-us to understand why something was done, basically :)
For example, the .env
change has nothing to do with developer flow. So that is confusing in a git blame
scenario.
So if you don't mind splitting this up in a few commits, that would be really appreciated :)
@@ -1,2 +1,3 @@ | |||
__pycache__/ | |||
*.pyc | |||
.env/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this /.env/
, as not to ignore any .env
in any folder :)
@@ -27,6 +27,8 @@ async def stop(self): | |||
|
|||
async def _read_response(self, response): | |||
if response.status in (200, 201, 400, 404): | |||
if response.content_type == "text/html": | |||
return 302, response.url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need a bit more detail what this does and why it is needed. For sure, if it is correct, it requires a comment in code explaining why this is there. But it feels off .. like something else is wrong :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With github mode, bananas-api will redirect the user to the login page:
https://github.com/OpenTTD/bananas-api/blob/7e9f4cf8c79cd6954607c8e43595d3c4d27e516e/bananas_api/user/github.py#L48
However, in developer mode, bananas-api will just return a login form:
https://github.com/OpenTTD/bananas-api/blob/7e9f4cf8c79cd6954607c8e43595d3c4d27e516e/bananas_api/user/developer.py#L16
The CLI tool expects to be redirected in case the request was succesful, so this is what is being simulated here.
The proper solution would be to also redirect in developer mode. Once I get more comfortable writing Python and with the bananas-api project I may be able to fix that, but not right now. I could create an issue in bananas-api and refer to that in a comment here perhaps?
Because of OpenTTD/bananas-api#88 this PR is no longer up to date, I'll send a new one somewhere next week when I have time to re-do this one taking the review comments into account. |
While setting up my dev environment for bananas, I had trouble authenticating to the API.
ape
, so I made that configurable as well.text/html
, I assume it should have been a 302 with the request URL instead. Parsing the response as json will definitely crash, so it will not deteriorate the experience very much. Ideally the bananas-api would actually return the 302, but on that side I did not see a straightforward way to make a difference between an automated request from the cli and a browser request (perhaps that will come as my python skills increase)..env
to.gitignore
for obvious reasons ;)