Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Upload: Clean up output. #283
Conversation
|
Nice work! I'll try and review soon; don't worry about the unit test dilemma; I have it covered |
elopio
reviewed
Jan 30, 2016
| @@ -47,7 +47,8 @@ def main(argv=None): | ||
| print('Enter your Ubuntu One SSO credentials.') | ||
| email = input('Email: ') | ||
| password = getpass.getpass('Password: ') | ||
| - otp = input('OTP: ') | ||
| + otp = input('One-time password (just press enter if you don\'t use ' | ||
| + 'two-factor authentication): ') |
kyrofa
Feb 1, 2016
Member
It's a bit verbose as we discussed, but I think it's a good intermediate solution until the uploader is smart enough to know whether or not it's actually required.
elopio
reviewed
Jan 30, 2016
| @@ -33,6 +38,7 @@ | ||
| ) | ||
| +logging.getLogger("requests").setLevel(logging.WARNING) |
elopio
Jan 30, 2016
Member
we generally call the logger file. Any special reason for using "requests" here instead?
kyrofa
Feb 1, 2016
Member
Ah, yeah notice I don't save the logger off here-- this line was for shutting up the requests lib. But perhaps this isn't the best place to put such a line?
elopio
reviewed
Jan 30, 2016
| @@ -48,7 +54,8 @@ def upload(binary_filename, metadata_filename='', metadata=None, config=None): | ||
| return | ||
| name = match.groupdict()['name'] | ||
| - logger.info('Uploading files...') | ||
| + logger.info('') |
elopio
Jan 30, 2016
Member
Why did you keep this one? I might be missing something, but I fail to see a reason for logging nothing :)
kyrofa
Feb 1, 2016
Member
This is only here to print a newline before the progress bar (it can't be contained within the progress bar or it keeps printing newlines). I just thought it looked a little nicer with some spacing.
elopio
reviewed
Jan 30, 2016
| if revision: | ||
| - logger.info('Uploaded as revision %s.', revision) | ||
| + message += ' (as revision {})'.format(revision) |
elopio
reviewed
Jan 30, 2016
| + # progresses. | ||
| + def upload_progress_callback(monitor): | ||
| + if monitor.bytes_read <= binary_file_size: | ||
| + progress_bar.update(monitor.bytes_read) |
elopio
Jan 30, 2016
Member
Could you extract a function for this block? maybe call it get_upload_progress_callback, that receives the binary_filename and returns the callback func.
elopio
reviewed
Jan 30, 2016
| - for fd in files.values(): | ||
| - fd.close() | ||
| + # Close the open file | ||
| + binary_file.close() |
elopio
Jan 30, 2016
Member
@sergiusens copied the same block in #285. I prefer your removal of the confusing dictionary of 1 element, so I think this one should land first.
sergiusens
Feb 1, 2016
Collaborator
Right, this may need to be refactored to support multiple snap uploads when library snaps come (hope we can have something for next week).
kyrofa
Feb 1, 2016
Member
Ah, yeah we'll have to discuss how we want to do the progress bar in that case.
|
Nice work indeed. |
kyrofa
referenced this pull request
Feb 1, 2016
Closed
Close fd on invalid credentials for upload #285
|
lgtm |
|
|
added a commit
that referenced
this pull request
Feb 1, 2016
sergiusens
merged commit 0c37853
into
snapcore:master
Feb 1, 2016
kyrofa
deleted the
kyrofa:bugfix/1538692/cleanup_upload_messages
branch
Feb 1, 2016
pushed a commit
to kalikiana/snapcraft
that referenced
this pull request
Apr 6, 2017
kyrofa commentedJan 29, 2016
This PR fixes LP: #1538692 by change the OTP prompt to be more clear, and adding progress indicators. The upload output now looks something like this:
Note that the test logger output is not cleaned here-- I bisected it back to 2329098 but I can't figure out what's causing it.