Push metadata to the Store. #1634

facundobatista · 2017-10-19T20:50:09Z

This is to fulfill first part of design discussed in this forum post.

kalikiana

@facundobatista Note the store tests are failing on Travis.

kalikiana · 2017-10-20T09:48:56Z

snapcraft/storeapi/__init__.py

@@ -364,6 +364,18 @@ def get_assertion(self, snap_id, endpoint):
    def sign_developer_agreement(self, latest_tos_accepted=False):
        return self.sca.sign_developer_agreement(latest_tos_accepted)

+    def update_metadata(self, snap_name, metadata, arch, force):


What's the purpose of arch here? It only seems to be used for the error.

Exactly! The idea is to maintain the detail level of the rest of SnapNotFoundError messages.

I see. It just strikes me as odd that the method would use a value in the error that isn't needed to do the actual work. As in, the error is providing more detail than is relevant to the failure. I might be missing something, though.

kalikiana · 2017-10-20T09:52:07Z

snapcraft/storeapi/errors.py

+            for field_name, error in sorted(conflicts):
+                sent = metadata.get(field_name)
+                parts.extend((
+                    "Conflict in {!r} field:".format(field_name),


The error should probably say something like You can use --force-metadata to update anyway?

And repeat this message in ALL conflict lines, you say?

Maybe it's better to add a line (in case of conflicts) after all conflicts, saying something like "You can repeat the command with --force-metadata to force the local values into the Store"?

What do you think? Thanks!

Yeah, that sounds great!

facundobatista · 2017-10-20T21:22:53Z

Oh, Travis failed because a CLA check: The command "./tools/travis/run_cla_check.sh" exited with 1., but I already signed the CLA.

Looks like failed the CLA check on @jocave ([email protected]), but I don't know why he is related to my branch :(

facundobatista · 2017-10-25T15:37:50Z

@kalikiana there I did the changes we talked about, thanks!

kalikiana

Grand, thanks! I like it!

come-maiz · 2017-10-26T03:29:52Z

snapcraft/_store.py

+
+    If only_metadata is True it will just send the metadata; otherwise the
+    whole process is done (including sending the metadata after sending the
+    snap binary).


Can you please document the force_metadata arg?

Done, thanks!

sergiusens · 2017-10-26T08:57:31Z

snapcraft/cli/store.py

    \b
    Examples:
        snapcraft push my-snap_0.1_amd64.snap
+        snapcraft push my-snap_0.1_amd64.snap --only-metadata


hey @matthewpaulthomas or @evandandrea do you think --only-metadata is correct here, it feels strange to me even though it is technically correct. Should this be --metadata or something else?

I don't think --only-metadata is self-explanatory. The option should reflect the developer's intention, not what happens internally (the metadata is sent, the snap is not).

I would be more explicit and use --overwrite-metadata.

Note however that "overwrite metadata" is not what it does, unless you also pass "force metadata", because pushing metadata can result in conflicts if you modified those fields through the web UI.

Also, these names are stablished in the proper forum post, should discuss these there?

sergiusens · 2017-10-26T08:58:00Z

snapcraft/cli/store.py

        snapcraft push my-snap_0.2_amd64.snap --release edge
        snapcraft push my-snap_0.3_amd64.snap --release candidate,beta
    """
+    if release and only_metadata:
+        raise click.UsageError(
+            "--only-metadata and --release are mutually exclusive")


Can't this be done with click itself?

I searched for this, it look it can't be done and no plans for click to gain this capability.

sergiusens · 2017-10-26T09:01:17Z

snapcraft/cli/store.py

@@ -115,7 +129,7 @@ def push(snap_file, release):
            'After pushing, an attempt to release to {} '
            'will be made'.format(channel_list))

-    snapcraft.push(snap_file, channel_list)
+    snapcraft.push(snap_file, channel_list, only_metadata, force_metadata)


instead of passing the logic over to push, can we handle the logic to call update_metadata from here? The printing with logger in that method should also be brought over here (it is a bit hard with some of the current logic we have but most of this stuff should be easily movable.

It can be moved, but conceptually "it's a push" (complete, or only metadata).

Another drawback of moving it to here is the need to call the private '_get_data_from_snap_file' from the private '_store".

I could start to move stuff around and/or make it public if you insist (but probably it would be better to do that in a separate branch, which I can commit to do if we decide this path)

facundobatista · 2017-10-27T11:22:10Z

Well, Travis now failed again with a weird message: "The job exceeded the maximum time limit for jobs, and has been terminated."

How can I trigger a new Travis check?

kalikiana · 2017-10-27T12:47:55Z

I re-triggered it for you.

facundobatista · 2017-10-27T19:51:00Z

Thanks! It worked ok now...

come-maiz · 2017-11-09T04:34:18Z

snapcraft/storeapi/errors.py

+
+    __FMT_NOT_REGISTERED = (
+        'Sorry, try `snapcraft register {snap_name}` before trying to '
+        'update metadata.')


I'm getting this error when I try to push --only-metadata without having pushed the snap first.
We should catch when the snapcraft is registered but not pushed, and recommend to call without --only-metadata. Is that possible?

Following @matthewpaulthomas's suggestion, the error messages should say what happened, why, and how to fix it.
What do you think of something like: "Failed to update metadata: snap not registered. Run first snapcraft register {snap_name} and then snapcraft push {snap_file}". ?

Will change, taking also Sergio's recommendation below

come-maiz · 2017-11-09T04:45:48Z

snapcraft/storeapi/errors.py

+                    "    In the Store:      {!r}".format(error['message']),
+                ))
+            parts.extend((
+                "You can repeat the command with --force-metadata",


This comma introduces a weird new line in the middle of the message. We generally don't wrap the messages, and if we do, we probably should do it in a single place. So, I think it would be better to add a single string.

Also related to this message, "You can repeat the command with --force-metadata" seems confusing when you get this error while pushing a new revision. Note that in that case the revision is correctly uploaded, so re-uploading it won't work (so you would need to do a --only-metadata + --force-metadata instead).

Regarding the comma, I was doing that on purpose, I'll fix it to have a single string.

And will improve the message.

come-maiz

@facundobatista sorry for the late late review.
This looks very good to me, I've just left a few comments, please let me know what do you think about them.

Also, it would be nice to have at least one integation_test for the happy update of the metadata. On that test it will be hard to verify if the update succeeded, which makes me think that we need a command to get the current values in the store. Something like snapcraft metadata <snap-name>.

Also it seems not possible to test the conflict in an integration_test against the staging server, right? In that case, documenting in manual_tests.md a test that makes a push, goes to the store, changes values, and tries to push again would be good.

sergiusens · 2017-11-09T12:48:23Z

On jue, nov 9, 2017 at 1:40 AM, Leo Arias ***@***.***> wrote: Following @matthewpaulthomas's suggestion, the error messages should say what happened, why, and how to fix it. What do you think of something like: "Failed to update metadata: snap not registered. Run first snapcraft register {snap_name} and thensnapcraft push {snap_file}". ?

Updating the metadata has failed: the snap is not registered. First run `snapcraft register <snap-name>` and the `snapcraft push <snap-file>`. We also need a new word for metadata as discussed on the forum topic for information extracion. Maybe "Updating the information on the store has failed"

facundobatista · 2017-11-10T19:26:40Z

@ElOpio thanks for the comments, there I answered them. Also I'll add an integration test, and describe in the manual file how to test the situation of the conflict (as you said, this can not be done automatically)

@sergiusens I'll like your suggestion for the message. Also will check that we don't say "metadata" in any human text

facundobatista · 2017-11-13T12:22:49Z

@ElOpio @sergiusens did the changes as discussed/reviewed, thanks!

sergiusens

Given this
"""
10:43 mpt Facu sorry my head works in mysterious ways... I don't know where I got my references from; but starting from scratch, does snapcraft push-metadata feel more straightforward and understandable than snapcraft push --only-metadata?
10:43 sergiusens, to me, yes
"""

I think we should just move over the command and make these minimal UI layer changes and we should be good to go

sergiusens · 2017-11-27T13:37:33Z

snapcraft/storeapi/__init__.py

@@ -364,6 +364,18 @@ def get_assertion(self, snap_id, endpoint):
    def sign_developer_agreement(self, latest_tos_accepted=False):
        return self.sca.sign_developer_agreement(latest_tos_accepted)

+    def update_metadata(self, snap_name, metadata, force):


From conversations on IRC, can you call this push_metadata?

sergiusens · 2017-11-27T13:37:52Z

snapcraft/storeapi/__init__.py

@@ -609,6 +621,21 @@ def snap_push_metadata(self, snap_name, updown_data,

        return StatusTracker(response.json()['status_details_url'])

+    def update_metadata(self, snap_id, snap_name, metadata, force):


Please rename to push_metadata

sergiusens

Thanks a but for your contribution! It looks really good and the team really appreciates your patience!

I have two minor comments about printing that should be super simple to address.

sergiusens · 2017-11-27T19:08:19Z

snapcraft/_store.py


-    logger.info("The metadata has been updated")
+    logger.info("The metadata has been pushed")


Just one minor nit, can you remove this from here and put it under

snapcraft.push_metadata(snap_file, force)

in snapcraft.cli.store?

sergiusens · 2017-11-27T19:09:54Z

snapcraft/_store.py

+    If force=True it will force the local metadata into the Store,
+    ignoring any possible conflict.
+    """
+    logger.info("Pushing metadata to the Store (force=%s)", force)


Mind removing this line or making it .debug or we will get a double print on regular runs?

facundobatista · 2017-11-27T20:35:59Z

Pushed requested change

Push metadata to the Store.

04bed64

kalikiana reviewed Oct 20, 2017

View reviewed changes

Changed the default behaviour of metadata endpoint in the fake server.

24c66f9

facundobatista added 2 commits October 25, 2017 09:02

Merge branch 'master' into push-metadata

110d289

Better message when conflict, and not use Arch as it's not needed.

36cf0b5

kalikiana approved these changes Oct 25, 2017

View reviewed changes

come-maiz reviewed Oct 26, 2017

View reviewed changes

sergiusens reviewed Oct 26, 2017

View reviewed changes

sergiusens added this to the 2.36 milestone Oct 26, 2017

Better docstring.

5a30b18

come-maiz reviewed Nov 9, 2017

View reviewed changes

Manual and integration tests, and better messages.

94cfca3

Merge branch 'master' into push-metadata

07d682c

sergiusens reviewed Nov 27, 2017

View reviewed changes

facundobatista added 3 commits November 27, 2017 10:50

Merge branch 'master' into push-metadata

6c1cff9

Changed paradigm to use other separate command, push-metadata.

6116594

Remove unused import.

0a4d4b0

sergiusens approved these changes Nov 27, 2017

View reviewed changes

Log in debug.

cef3d4f

sergiusens merged commit 2f61ad0 into canonical:master Nov 27, 2017

		@@ -609,6 +621,21 @@ def snap_push_metadata(self, snap_name, updown_data,

		return StatusTracker(response.json()['status_details_url'])

		def update_metadata(self, snap_id, snap_name, metadata, force):


		logger.info("The metadata has been updated")
		logger.info("The metadata has been pushed")

Push metadata to the Store. #1634

Push metadata to the Store. #1634

Conversation

facundobatista commented Oct 19, 2017

kalikiana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facundobatista commented Oct 20, 2017

facundobatista commented Oct 25, 2017

kalikiana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facundobatista commented Oct 27, 2017

kalikiana commented Oct 27, 2017

facundobatista commented Oct 27, 2017

Choose a reason for hiding this comment

come-maiz Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

come-maiz left a comment

Choose a reason for hiding this comment

sergiusens commented Nov 9, 2017 via email

facundobatista commented Nov 10, 2017

facundobatista commented Nov 13, 2017

sergiusens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiusens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facundobatista commented Nov 27, 2017

come-maiz Nov 9, 2017 •

edited

Loading