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

Combined PR to fix issues #3, #4, #5 and PR #20 #23

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

erenes
Copy link

@erenes erenes commented Apr 10, 2021

When submitting my previous pull requests, I took care to keep commits for separate fixes separate from each other so they would not lead to merge conflicts.

Unfortunately, that completely backfired and lead to merge conflicts when I wanted to merge them locally and actually use bananas-cli.

In this PR I have resolved the merge conflicts, and additionally fixed issue #3.
For review purposes, I have left PR #20, #21, and #22 up, but to avoid you having to deal with the same merge errors upstream, I recommend merging only this PR instead of each PR separately.

If this PR is merged I'll close the other ones. If you prefer to review the whole bunch at once you're welcome to close the other PRs before this is merged :)


For issue #3 I have tested and handled the following scenarios:

  • Wrong api url
  • Wrong tus url
  • No files passed for upload

Additionally I have added the --verbose flag, which can be used if stacktraces in python are exactly what you are looking for.

if self.verbose:
log.exception(f"Failed to upload file '{filename}'")
else:
log.error(f"Failed to upload file '{filename}': {e}")
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this gives better errors that the user understands sufficiently. Do you have an example of this? In my experience, the str(e) often gives something impossible to understand :D

Copy link
Author

Choose a reason for hiding this comment

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

My first iteration just said "failed to upload file {filename}", but that took away all agency from me as a command line user. I think if you are using the command line, you probably are more comfortable understanding error messages than a clicky person.

Aanyway:
If not specifying TUS url:
2021-04-12 17:21:01 ERROR Failed to upload file 'xxxx.grf': Attempt to retrieve create file url with status 404

With --verbose specified:

2021-04-12 17:22:19 ERROR    Failed to upload file 'xxxx.grf'
Traceback (most recent call last):
  File "/code/bananas-frontend-cli/bananas_cli/session.py", line 62, in tus_upload
    uploader = tus.uploader(
  File "/code/bananas-frontend-cli/.env/lib/python3.8/site-packages/tusclient/client.py", line 52, in uploader
    return Uploader(*args, **kwargs)
  File "/code/bananas-frontend-cli/.env/lib/python3.8/site-packages/tusclient/uploader.py", line 124, in __init__
    self.url = url or self.get_url()
  File "/code/bananas-frontend-cli/.env/lib/python3.8/site-packages/tusclient/uploader.py", line 216, in get_url
    return self.create_url()
  File "/code/bananas-frontend-cli/.env/lib/python3.8/site-packages/tusclient/uploader.py", line 22, in _wrapper
    return func(*args, **kwargs)
  File "/code/bananas-frontend-cli/.env/lib/python3.8/site-packages/tusclient/uploader.py", line 232, in create_url
    raise TusCommunicationError(msg, resp.status_code, resp.content)
tusclient.exceptions.TusCommunicationError: Attempt to retrieve create file url with status 404

Typing https instead of http:
2021-04-12 17:23:49 ERROR Failed to upload file 'v9point8.grf': HTTPSConnectionPool(host='172.17.0.2', port=1080): Max retries exceeded with url: /new-package/tus/ (Caused by SSLError(SSLError(1, '[SSL: WRONG_VERSION_NUMBER] wrong version number (_ssl.c:1125)'))

Wrong api url:
2021-04-12 17:28:29 ERROR Cannot connect to host 172.17.0.2:443 ssl:default [Connect call failed ('172.17.0.2', 443)]

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.

2 participants