commands: fix missing built_at store upload option (#4261)

snapcraft-started-at is a manifest field. This suggests that upload
never sent built_at because it was trying to extract it from
meta/snap.yaml instead of snap/manifest.yaml.

The unused before this change test file test-snap-with-started-at.snap
was updated accordingly:

- snapcraft-started-at field was removed from meta/snap.yaml,
- and a new snap/manifest.yaml was added.

The change was tested against the Store.

Co-authored-by: Sergio Schvezov <sergio.schvezov@canonical.com>
Co-authored-by: Callahan <callahan.kovacs@canonical.com>

snapcraft/commands/upload.py

@@ -80,9 +80,11 @@ def run(self, parsed_args):
 
         client = store.StoreClientCLI()
 
-        snap_yaml = get_data_from_snap_file(snap_file)
+        snap_yaml, manifest_yaml = get_data_from_snap_file(snap_file)
         snap_name = snap_yaml["name"]
-        built_at = snap_yaml.get("snapcraft-started-at")
+        built_at = None
+        if manifest_yaml:
+            built_at = manifest_yaml.get("snapcraft-started-at")
 
         client.verify_upload(snap_name=snap_name)
 

snapcraft_legacy/_store.py

@@ -52,6 +52,7 @@
 
 
 def get_data_from_snap_file(snap_path):
+    manifest_yaml = None
     with tempfile.TemporaryDirectory() as temp_dir:
         unsquashfs_path = get_snap_tool_path("unsquashfs")
         try:
@@ -61,9 +62,9 @@ def get_data_from_snap_file(snap_path):
                     "-d",
                     os.path.join(temp_dir, "squashfs-root"),
                     snap_path,
-                    "-e",
                     # cygwin unsquashfs on windows uses unix paths.
                     Path("meta", "snap.yaml").as_posix(),
+                    Path("snap", "manifest.yaml").as_posix(),
                 ]
             )
         except subprocess.CalledProcessError:
@@ -73,7 +74,12 @@ def get_data_from_snap_file(snap_path):
             os.path.join(temp_dir, "squashfs-root", "meta", "snap.yaml")
         ) as yaml_file:
             snap_yaml = yaml_utils.load(yaml_file)
-    return snap_yaml
+        manifest_path = Path(temp_dir, "squashfs-root", "snap", "manifest.yaml")
+        if manifest_path.exists():
+            with open(manifest_path) as manifest_yaml_file:
+                manifest_yaml = yaml_utils.load(manifest_yaml_file)
+
+    return snap_yaml, manifest_yaml
 
 
 @contextlib.contextmanager
@@ -584,7 +590,7 @@ def sign_build(snap_filename, key_name=None, local=False):
     if not os.path.exists(snap_filename):
         raise FileNotFoundError("The file {!r} does not exist.".format(snap_filename))
 
-    snap_yaml = get_data_from_snap_file(snap_filename)
+    snap_yaml, _ = get_data_from_snap_file(snap_filename)
     snap_name = snap_yaml["name"]
     grade = snap_yaml.get("grade", "stable")
 
@@ -635,7 +641,7 @@ def upload_metadata(snap_filename, force):
     logger.debug("Uploading metadata to the Store (force=%s)", force)
 
     # get the metadata from the snap
-    snap_yaml = get_data_from_snap_file(snap_filename)
+    snap_yaml, _ = get_data_from_snap_file(snap_filename)
     metadata = {
         "summary": snap_yaml["summary"],
         "description": snap_yaml["description"],

tests/legacy/unit/commands/test_sign_build.py

@@ -81,7 +81,7 @@ def test_sign_build_missing_account_info(
         self,
         mock_get_snap_data,
     ):
-        mock_get_snap_data.return_value = {"name": "test-snap", "grade": "stable"}
+        mock_get_snap_data.return_value = {"name": "test-snap", "grade": "stable"}, None
 
         raised = self.assertRaises(
             storeapi.errors.StoreBuildAssertionPermissionError,
@@ -104,7 +104,7 @@ def test_sign_build_no_usable_keys(
         self,
         mock_get_snap_data,
     ):
-        mock_get_snap_data.return_value = {"name": "snap-test", "grade": "stable"}
+        mock_get_snap_data.return_value = {"name": "snap-test", "grade": "stable"}, None
 
         self.useFixture(
             fixtures.MockPatch("subprocess.check_output", return_value="[]".encode())
@@ -138,7 +138,7 @@ def test_sign_build_no_usable_named_key(
             "account_id": "abcd",
             "snaps": {"16": {"test-snap": {"snap-id": "snap-id"}}},
         }
-        mock_get_snap_data.return_value = {"name": "test-snap", "grade": "stable"}
+        mock_get_snap_data.return_value = {"name": "test-snap", "grade": "stable"}, None
         self.useFixture(
             fixtures.MockPatch(
                 "subprocess.check_output", return_value='[{"name": "default"}]'.encode()
@@ -172,7 +172,7 @@ def test_sign_build_unregistered_key(
             "account_keys": [{"public-key-sha3-384": "another_hash"}],
             "snaps": {"16": {"test-snap": {"snap-id": "snap-id"}}},
         }
-        mock_get_snap_data.return_value = {"name": "test-snap", "grade": "stable"}
+        mock_get_snap_data.return_value = {"name": "test-snap", "grade": "stable"}, None
         self.useFixture(
             fixtures.MockPatch(
                 "subprocess.check_output",
@@ -210,7 +210,7 @@ def test_sign_build_locally_successfully(
             "account_id": "abcd",
             "snaps": {"16": {"test-snap": {"snap-id": "snap-id"}}},
         }
-        mock_get_snap_data.return_value = {"name": "test-snap", "grade": "stable"}
+        mock_get_snap_data.return_value = {"name": "test-snap", "grade": "stable"}, None
         fake_check_output = fixtures.MockPatch(
             "subprocess.check_output",
             side_effect=mock_check_output,
@@ -253,7 +253,7 @@ def test_sign_build_missing_grade(
             "account_keys": [{"public-key-sha3-384": "a_hash"}],
             "snaps": {"16": {"test-snap": {"snap-id": "snap-id"}}},
         }
-        mock_get_snap_data.return_value = {"name": "test-snap"}
+        mock_get_snap_data.return_value = {"name": "test-snap"}, None
         fake_check_output = fixtures.MockPatch(
             "subprocess.check_output", side_effect=mock_check_output
         )
@@ -301,7 +301,7 @@ def test_sign_build_upload_successfully(
             ],
             "snaps": {"16": {"test-snap": {"snap-id": "snap-id"}}},
         }
-        mock_get_snap_data.return_value = {"name": "test-snap", "grade": "stable"}
+        mock_get_snap_data.return_value = {"name": "test-snap", "grade": "stable"}, None
         fake_check_output = fixtures.MockPatch(
             "subprocess.check_output",
             side_effect=mock_check_output,
@@ -350,7 +350,7 @@ def test_sign_build_upload_existing(
             "account_id": "abcd",
             "snaps": {"16": {"test-snap": {"snap-id": "snap-id"}}},
         }
-        mock_get_snap_data.return_value = {"name": "test-snap", "grade": "stable"}
+        mock_get_snap_data.return_value = {"name": "test-snap", "grade": "stable"}, None
 
         snap_build_path = self.snap_test.snap_path + "-build"
         with open(snap_build_path, "wb") as fd:

tests/unit/commands/test_upload.py

@@ -57,6 +57,20 @@ def snap_file():
     )
 
 
+@pytest.fixture
+def snap_file_with_started_at():
+    return str(
+        (
+            pathlib.Path(unit.__file__)
+            / ".."
+            / ".."
+            / "legacy"
+            / "data"
+            / "test-snap-with-started-at.snap"
+        ).resolve()
+    )
+
+
 ##################
 # Upload Command #
 ##################
@@ -96,6 +110,40 @@ def test_default(
     emitter.assert_message("Revision 10 created for 'basic'")
 
 
+@pytest.mark.usefixtures("memory_keyring")
+@pytest.mark.parametrize(
+    "command_class", (commands.StoreUploadCommand, commands.StoreLegacyPushCommand)
+)
+def test_built_at(
+    emitter,
+    fake_store_notify_upload,
+    fake_store_verify_upload,
+    snap_file_with_started_at,
+    command_class,
+):
+    cmd = command_class(None)
+
+    cmd.run(
+        argparse.Namespace(
+            snap_file=snap_file_with_started_at,
+            channels=None,
+        )
+    )
+
+    assert fake_store_verify_upload.mock_calls == [call(ANY, snap_name="basic")]
+    assert fake_store_notify_upload.mock_calls == [
+        call(
+            ANY,
+            snap_name="basic",
+            upload_id="2ecbfac1-3448-4e7d-85a4-7919b999f120",
+            built_at="2019-05-07T19:25:53.939041Z",
+            channels=None,
+            snap_file_size=4096,
+        )
+    ]
+    emitter.assert_message("Revision 10 created for 'basic'")
+
+
 @pytest.mark.usefixtures("memory_keyring")
 def test_default_channels(
     emitter, fake_store_notify_upload, fake_store_verify_upload, snap_file