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
store: implement push pre-check. #1009
Conversation
Current coverage is 96.34% (diff: 84.21%)@@ master #1009 diff @@
==========================================
Files 194 194
Lines 17251 17286 +35
Methods 0 0
Messages 0 0
Branches 1338 1339 +1
==========================================
+ Hits 16625 16654 +29
- Misses 425 431 +6
Partials 201 201
|
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 looks good, just a quick question.
snap_yaml = _get_data_from_snap_file(snap_filename) | ||
snap_name = snap_yaml['name'] | ||
store = storeapi.StoreClient() | ||
|
||
with _requires_login(): | ||
store.push_precheck(snap_name) |
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.
Does this step take any significant amount of time? Should a message be printed about what it's doing in case it hangs at all?
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.
or do this after logging the message for which I want to change anyways ;-)
@kyrofa It does require an http request, which could potentially timeout if there's
an issue with the store. Logging something might be useful, but I'm not
sure what the best way to convey what the 'precheck' is for to the user.
Any suggestions? Maybe "Checking snap status in store..."?
…On Wed, 21 Dec 2016 at 5:27 AM, Kyle Fazzari ***@***.***> wrote:
***@***.**** commented on this pull request.
This looks good, just a quick question.
------------------------------
In snapcraft/_store.py
<#1009 (review)>
:
> snap_yaml = _get_data_from_snap_file(snap_filename)
snap_name = snap_yaml['name']
store = storeapi.StoreClient()
+
+ with _requires_login():
+ store.push_precheck(snap_name)
Does this step take any significant amount of time? Should a message be
printed about what it's doing in case it hangs at all?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1009 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAH87oWCV3tpOUpsgdACn2kizy4RbT-Fks5rKAILgaJpZM4LRdZu>
.
|
Yeah that's fine, or even as opaque as "Checking with the store..." would be fine. The reason here is I think twofold:
|
with _requires_login(): | ||
store.push_precheck(snap_name) | ||
|
||
logger.info('Uploading {}.'.format(snap_filename)) |
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.
can we take the opportunity to call change this to 'Pushing {!r}'
?
LP: #1610776
Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
b69f4f0
to
62ccccd
Compare
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.
Yeah that seems fine.
LP: #1610776 Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
LP: #1610776
This branch adds a precheck request to the store to
snapcraft push
, which in the case of an unregistered snap will report the error to the user before attempting a full upload.