Merge ~suligap/snapstore-client:airgap-related-bugfixes into snapstore-client:master

Proposed by Przemysław Suliga on 2020-04-09
Status: Merged
Approved by: Przemysław Suliga on 2020-04-09
Approved revision: 9c22a4221a0a608f0bf150a9c0434ba8119c8d61
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~suligap/snapstore-client:airgap-related-bugfixes
Merge into: snapstore-client:master
Diff against target: 179 lines (+71/-13)
3 files modified
snapstore_client/logic/push.py (+29/-6)
snapstore_client/logic/tests/test-snap-assert.assert (+1/-1)
snapstore_client/logic/tests/test_push.py (+41/-6)
Reviewer Review Type Date Requested Status
Simon Davy (community) 2020-04-09 Approve on 2020-04-09
Review via email: mp+381974@code.launchpad.net

Commit message

Fixes for remote airgap snap uploads

- missing utf-8 encoding of assertions payload
- ignore missing revisions when uploading snap files
- missing package_type field in various API calls

Description of the change

Tested this locally against an airgap proxy built from recent snapstore-snap master.

To post a comment you must log in.
Simon Davy (bloodearnest) wrote :

LGTM. If you think snap-store-proxy has not had attention, the client is worse :(

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/snapstore_client/logic/push.py b/snapstore_client/logic/push.py
2index abd06e8..1a91bbb 100644
3--- a/snapstore_client/logic/push.py
4+++ b/snapstore_client/logic/push.py
5@@ -40,6 +40,7 @@ def _push_ident(store, password, downloaded_map):
6 snap_details = downloaded_map['snap']
7 push_details = {
8 'snap_id': snap_id,
9+ 'package_type': 'snap',
10 'snap_name': snap_details['name'],
11 'private': False, # If you're pushing to your own proxy
12 'stores': ['ubuntu'], # ditto
13@@ -84,6 +85,7 @@ def _push_revs(store, password, downloaded_map):
14 # create the revision
15 rev = {
16 'snap_id': snap_id,
17+ 'package_type': 'snap',
18 'revision': instance['revision'],
19 'version': instance['version'],
20 'confinement': instance['confinement'],
21@@ -118,10 +120,19 @@ def _push_channelmap(store, password, downloaded_map, force_push=False):
22
23 snap_id = downloaded_map['snap-id']
24
25+ filter_payload = {
26+ 'filters': [
27+ {
28+ 'series': '16',
29+ 'package_type': 'snap',
30+ 'snap_id': snap_id,
31+ },
32+ ],
33+ }
34 # Check if we have an existing channel map
35 existing = requests.post(
36 store_url + '/channelmaps/filter',
37- json={'filters': [{'series': '16', 'snap_id': snap_id}]},
38+ json=filter_payload,
39 auth=requests.auth.HTTPBasicAuth(USERNAME, password))
40 if existing.status_code != 200:
41 raise pushException("Error retrieving current channelmap")
42@@ -140,6 +151,7 @@ def _push_channelmap(store, password, downloaded_map, force_push=False):
43 'developer_id': downloaded_map['snap']['publisher']['id'],
44 'release_requests': [
45 {
46+ 'package_type': 'snap',
47 'snap_id': snap_id,
48 'channel': [
49 track,
50@@ -204,7 +216,7 @@ def _add_assertion_to_service(store, password, list_assertions):
51 continue
52 response = requests.post(
53 store_url + '/v1/assertions',
54- '\n'.join(assertion),
55+ '\n'.join(assertion).encode('utf-8'),
56 headers={'Content-Type': 'application/x.ubuntu.assertion'},
57 auth=requests.auth.HTTPBasicAuth(USERNAME, password))
58 if response.status_code != 201:
59@@ -278,10 +290,21 @@ def _push(store, tar_file, password, force_channel_map=False):
60 snap_name, instance['revision'])
61 snap_path = str(json_path.parent / snap_file_name)
62 assert_path = str(json_path.parent / assert_file_name)
63- _push_file_to_nginx_cache(
64- store, password, snap_path,
65- downloaded_map['snap-id'], instance['revision'])
66- _push_assertions(store, password, assert_path)
67+ try:
68+ _push_file_to_nginx_cache(
69+ store, password, snap_path,
70+ downloaded_map['snap-id'], instance['revision'],
71+ )
72+ except FileNotFoundError as exc:
73+ logger.warning(
74+ 'Skipping revision %s (%s/%s) as %s not found',
75+ instance['revision'],
76+ instance['channel']['name'],
77+ instance['channel']['architecture'],
78+ exc.filename,
79+ )
80+ else:
81+ _push_assertions(store, password, assert_path)
82
83
84 def push_snap(args):
85diff --git a/snapstore_client/logic/tests/test-snap-assert.assert b/snapstore_client/logic/tests/test-snap-assert.assert
86index b6b2ebd..4eda67a 100644
87--- a/snapstore_client/logic/tests/test-snap-assert.assert
88+++ b/snapstore_client/logic/tests/test-snap-assert.assert
89@@ -42,7 +42,7 @@ oG3Ie3WOHrVjCLXIdYslpL1O4nadqR6Xv58pHj6k
90 type: account
91 authority-id: canonical
92 account-id: 1RgHGj7Zp3nyycrGa5sODDbZznzJAwAX
93-display-name: Tom Wardill
94+display-name: Tom Wardill (Ω)
95 timestamp: 2018-02-06T09:22:49.118291Z
96 username: twom
97 validation: unproven
98diff --git a/snapstore_client/logic/tests/test_push.py b/snapstore_client/logic/tests/test_push.py
99index 7fedbbd..cda240c 100644
100--- a/snapstore_client/logic/tests/test_push.py
101+++ b/snapstore_client/logic/tests/test_push.py
102@@ -37,9 +37,15 @@ class pushTests(TestCase):
103
104 _push_ident(self.store, 'test', self.snap_map)
105
106+ request = responses.calls[0].request
107+ payload = json.loads(request.body.decode('utf-8'))
108+
109 self.assertEqual(
110- 'Basic YWRtaW46dGVzdA==',
111- responses.calls[0].request.headers['Authorization'])
112+ payload['snaps'][0]['package_type'], 'snap',
113+ )
114+ self.assertEqual(
115+ 'Basic YWRtaW46dGVzdA==', request.headers['Authorization'],
116+ )
117
118 @responses.activate
119 def test_push_ident_failed(self):
120@@ -56,9 +62,13 @@ class pushTests(TestCase):
121
122 _push_revs(self.store, 'test', self.snap_map)
123
124+ request = responses.calls[0].request
125+ payload = json.loads(request.body.decode('utf-8'))
126+
127+ self.assertEqual(payload[0]['package_type'], 'snap')
128 self.assertEqual(
129- 'Basic YWRtaW46dGVzdA==',
130- responses.calls[0].request.headers['Authorization'])
131+ 'Basic YWRtaW46dGVzdA==', request.headers['Authorization'],
132+ )
133
134 @responses.activate
135 def test_push_revs_unexpected_status_code(self):
136@@ -85,9 +95,30 @@ class pushTests(TestCase):
137
138 _push_channelmap(self.store, 'test', self.snap_map)
139
140+ filter_request = responses.calls[0].request
141+ filter_payload = json.loads(filter_request.body.decode('utf-8'))
142 self.assertEqual(
143- 'Basic YWRtaW46dGVzdA==',
144- responses.calls[0].request.headers['Authorization'])
145+ filter_payload['filters'],
146+ [
147+ {
148+ 'series': '16',
149+ 'package_type': 'snap',
150+ 'snap_id': self.snap_map['snap-id'],
151+ },
152+ ],
153+ )
154+ self.assertEqual(
155+ 'Basic YWRtaW46dGVzdA==', filter_request.headers['Authorization'],
156+ )
157+
158+ chanmap_update_request = responses.calls[1].request
159+ chanmap_update_payload = json.loads(
160+ chanmap_update_request.body.decode('utf-8'),
161+ )
162+ self.assertEqual(
163+ chanmap_update_payload['release_requests'][0]['package_type'],
164+ 'snap',
165+ )
166
167 @responses.activate
168 def test_push_map_failed(self):
169@@ -144,6 +175,10 @@ class pushTests(TestCase):
170 split_assertions = _split_assertions(self.snap_assert)
171 _add_assertion_to_service(self.store, 'test', split_assertions)
172
173+ self.assertIn(
174+ 'display-name: Tom Wardill (Ω)',
175+ responses.calls[0].request.body.decode('utf-8'),
176+ )
177 self.assertEqual(
178 'Basic YWRtaW46dGVzdA==',
179 responses.calls[0].request.headers['Authorization'])

Subscribers

People subscribed via source and target branches

to all changes: