Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
storeapi: support for pushing binary metadata #1774
Conversation
matiasb
added some commits
Nov 28, 2017
sergiusens
added this to the 2.37 milestone
Nov 29, 2017
kyrofa
reviewed
Nov 30, 2017
This looks mostly good, though I have a question and it seems perhaps one test case isn't there. I need to give this a more thorough test tomorrow.
| + ['unsquashfs', '-d', | ||
| + os.path.join(temp_dir, 'squashfs-root'), | ||
| + snap_path, '-e', 'meta/gui']) | ||
| + logger.debug(output) |
kyrofa
Nov 30, 2017
Member
I suspect this debug output will not be very useful on its own. Would it be worth adding some context to it?
| @@ -404,6 +425,9 @@ def push_metadata(snap_filename, force): | ||
| with _requires_login(): | ||
| store.push_precheck(snap_name) | ||
| store.push_metadata(snap_name, metadata, force) | ||
| + with _get_icon_from_snap_file(snap_filename) as icon: | ||
| + metadata = {'icon': icon} | ||
| + store.push_binary_metadata(snap_name, metadata, force) |
kyrofa
Nov 30, 2017
Member
It's possible for the icon to be None, here. I can't tell from the below: will that remove the icon from the store? That situation does not seem covered by tests.
matiasb
Nov 30, 2017
Contributor
Yes, icon could be None, and that will remove the icon from the store.
I'll review tests and add it if missing. Thanks!
|
Thanks for the additional test, @matiasb. I've also tested this against the staging store with success. However, is there no way to selectively update metadata without updating it all in one go? What if I only wanted to update the icon, but not the other metadata? |
|
The implementation is the result of the discussion in https://forum.snapcraft.io/t/mechanisms-for-converging-store-and-snap-metadata/2200 (that summarizes the discussions during the rally too). |
sergiusens
changed the title from
Push binary metadata to the Store
to
storeapi: support for pushing binary metadata
Dec 1, 2017
| + | ||
| +# Test push binary metadata with conflicts | ||
| + | ||
| +1. 'snapcraft build' a simple snap |
| +2. Do a simple 'snapcraft push SNAP' | ||
| +3. Go to the Web and change snap's icon | ||
| +4. Change the snap's icon in the YAML file to something different than you put in the Web | ||
| +5. Try to update snap's metadata using `snapcraft push-metadata SNAP` |
matiasb
Dec 4, 2017
Contributor
Push won't raise the conflict, and it won't update the metadata either. Metadata from a pushed revision will only be used if there are no previous changes made in the web UI.
Should we think about adding some warning/message while processing a push about conflicting metadata?
sergiusens
Dec 4, 2017
Collaborator
We shouldn't block on this for the PR, but an error code saying the metadata is not being updated due to divergences for which we can on the client instruct the user would be good. Being silent by default is fine.
| + icon_file = open(icon_path, 'rb') | ||
| + break | ||
| + yield icon_file | ||
| + if icon_file is not None: |
sergiusens
Dec 1, 2017
Collaborator
should the yield above be wrapped in between a try and finally to always check and close the icon handle?
| + | ||
| + files = {} | ||
| + # only icon support atm | ||
| + icon = metadata.get('icon') |
sergiusens
Dec 1, 2017
Collaborator
this is going to end up being a rather big function, how about a private module inside storeapi to handle this with some OO? Nothing complex
what do you think?
sergiusens
requested changes
Dec 1, 2017
Looks really good, just have comments/questions to throw back at you.
|
works great |
sergiusens
merged commit d549249
into
snapcore:master
Dec 5, 2017
1 check passed
added a commit
to gsilvapt/snapcraft
that referenced
this pull request
Dec 8, 2017
added a commit
to daniellimws/snapcraft
that referenced
this pull request
Dec 14, 2017
matiasb commentedNov 29, 2017
Update push-metadata command to also push binary metadata (so far, icon only).