Merge ~jslarraz/review-tools:use-logging-module into review-tools:master
- Git
- lp:~jslarraz/review-tools
- use-logging-module
- Merge into master
Proposed by
Jorge Sancho Larraz
Status: | Merged |
---|---|
Merged at revision: | f2c630c4f9025a91435ee9a60d93b382756c13e4 |
Proposed branch: | ~jslarraz/review-tools:use-logging-module |
Merge into: | review-tools:master |
Diff against target: |
1149 lines (+150/-212) 17 files modified
bin/create-snap-declaration (+3/-3) bin/dump-tool (+9/-9) bin/rock-check-notices (+6/-7) bin/rock-updates-available (+7/-7) bin/snap-check-notices (+6/-7) bin/snap-review (+8/-18) bin/snap-updates-available (+10/-10) bin/snap-verify-declaration (+8/-11) bin/store-query (+11/-9) reviewtools/__init__.py (+16/-0) reviewtools/available.py (+5/-7) reviewtools/common.py (+17/-58) reviewtools/sr_declaration.py (+4/-3) reviewtools/store.py (+26/-27) tests/test-dump-tool.sh.expected (+8/-8) tests/test-snap-verify-declaration.sh.expected (+3/-25) tests/test-updates-available.sh.expected (+3/-3) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Alex Murray | Approve | ||
Review via email:
|
Commit message
Use python logging module instead of custom error, warn, msg and debug functions defined in reviewtools.common
Description of the change
To post a comment you must log in.
Revision history for this message
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Jorge Sancho Larraz (jslarraz) wrote : | # |
Revision history for this message
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Jorge Sancho Larraz (jslarraz) : | # |
Revision history for this message
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Alex Murray (alexmurray) wrote : | # |
Thanks for the very extensive comments to help walk-through this MR - it was very valuable. I really like this cleanup and it makes the code more pythonic by using the standard logging module, plus I actually like the CLI printing the errors directly rather than including them in the json output. LGTM!
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/bin/create-snap-declaration b/bin/create-snap-declaration | |||
2 | index 4666a65..a924e93 100755 | |||
3 | --- a/bin/create-snap-declaration | |||
4 | +++ b/bin/create-snap-declaration | |||
5 | @@ -5,6 +5,7 @@ import json | |||
6 | 5 | import re | 5 | import re |
7 | 6 | import sys | 6 | import sys |
8 | 7 | 7 | ||
9 | 8 | from reviewtools import logger | ||
10 | 8 | from reviewtools.common import read_snapd_base_declaration | 9 | from reviewtools.common import read_snapd_base_declaration |
11 | 9 | 10 | ||
12 | 10 | decl = {} | 11 | decl = {} |
13 | @@ -158,9 +159,8 @@ def add_missing(): | |||
14 | 158 | 159 | ||
15 | 159 | cstr_s = "allow-%s" % cstr | 160 | cstr_s = "allow-%s" % cstr |
16 | 160 | if in_base and cstr_s not in decl[side][iface]: | 161 | if in_base and cstr_s not in decl[side][iface]: |
20 | 161 | print( | 162 | logger.warning( |
21 | 162 | "WARN: adding missing '%s' for '%s' from base decl" | 163 | "adding missing '%s' for '%s' from base decl" % (cstr, iface) |
19 | 163 | % (cstr, iface) | ||
22 | 164 | ) | 164 | ) |
23 | 165 | decl[side][iface][cstr_b] = bool2str(base_decl[side][iface][cstr_b]) | 165 | decl[side][iface][cstr_b] = bool2str(base_decl[side][iface][cstr_b]) |
24 | 166 | if isinstance(decl[side][iface][cstr_b], dict): | 166 | if isinstance(decl[side][iface][cstr_b], dict): |
25 | diff --git a/bin/dump-tool b/bin/dump-tool | |||
26 | index 1361328..4a9658b 100755 | |||
27 | --- a/bin/dump-tool | |||
28 | +++ b/bin/dump-tool | |||
29 | @@ -57,7 +57,7 @@ import textwrap | |||
30 | 57 | import yaml | 57 | import yaml |
31 | 58 | 58 | ||
32 | 59 | import reviewtools.common as common | 59 | import reviewtools.common as common |
34 | 60 | from reviewtools.common import error, warn, msg, debug | 60 | from reviewtools import logger |
35 | 61 | 61 | ||
36 | 62 | # TODO: | 62 | # TODO: |
37 | 63 | # - --file read in one at a time and flush to disk (either --output or | 63 | # - --file read in one at a time and flush to disk (either --output or |
38 | @@ -72,11 +72,11 @@ def _read_store_dump(input, warn_if_empty=False, progress=False, merge=None): | |||
39 | 72 | """ | 72 | """ |
40 | 73 | 73 | ||
41 | 74 | def _add_entry(db, id, rev, y): | 74 | def _add_entry(db, id, rev, y): |
43 | 75 | debug("adding: id=%s,rev=%s,yaml=\n%s" % (id, rev, y)) | 75 | logger.debug("adding: id=%s,rev=%s,yaml=\n%s" % (id, rev, y)) |
44 | 76 | try: | 76 | try: |
45 | 77 | snap_yaml = yaml.load(y, Loader=yaml.SafeLoader) | 77 | snap_yaml = yaml.load(y, Loader=yaml.SafeLoader) |
46 | 78 | except Exception as e: | 78 | except Exception as e: |
48 | 79 | warn("Skipping %s|%s: %s" % (cId, cRev, e)) | 79 | logger.warning("Skipping %s|%s: %s" % (cId, cRev, e)) |
49 | 80 | return | 80 | return |
50 | 81 | 81 | ||
51 | 82 | # skipping existing entries | 82 | # skipping existing entries |
52 | @@ -115,7 +115,7 @@ def _read_store_dump(input, warn_if_empty=False, progress=False, merge=None): | |||
53 | 115 | (cId, cRev, cYaml) = line.split("|") | 115 | (cId, cRev, cYaml) = line.split("|") |
54 | 116 | if cYaml == "\n": # id|rev| | 116 | if cYaml == "\n": # id|rev| |
55 | 117 | if warn_if_empty and not progress: | 117 | if warn_if_empty and not progress: |
57 | 118 | warn("Skipping %s|%s: empty" % (cId, cRev)) | 118 | logger.warning("Skipping %s|%s: empty" % (cId, cRev)) |
58 | 119 | cId = None | 119 | cId = None |
59 | 120 | elif cId is not None: | 120 | elif cId is not None: |
60 | 121 | cYaml += "%s" % (line) | 121 | cYaml += "%s" % (line) |
61 | @@ -139,7 +139,7 @@ def _write_json_db(db, fn, stdout=False): | |||
62 | 139 | return | 139 | return |
63 | 140 | 140 | ||
64 | 141 | if os.path.exists(fn): | 141 | if os.path.exists(fn): |
66 | 142 | error("%s exists. Aborting" % (fn)) | 142 | logger.error("%s exists. Aborting" % (fn)) |
67 | 143 | with open(fn, "w") as fh: | 143 | with open(fn, "w") as fh: |
68 | 144 | json.dump(db, fh, sort_keys=True, indent=2) | 144 | json.dump(db, fh, sort_keys=True, indent=2) |
69 | 145 | fh.write("\n") | 145 | fh.write("\n") |
70 | @@ -183,21 +183,21 @@ def main(): | |||
71 | 183 | 183 | ||
72 | 184 | # Arg validation | 184 | # Arg validation |
73 | 185 | if not (args.file) and not (args.db_file): | 185 | if not (args.file) and not (args.db_file): |
75 | 186 | error("Must specify either --file or --db-file") | 186 | logger.error("Must specify either --file or --db-file") |
76 | 187 | elif (args.file) and (args.db_file): | 187 | elif (args.file) and (args.db_file): |
77 | 188 | if not os.path.exists(args.file) or not os.path.exists(args.db_file): | 188 | if not os.path.exists(args.file) or not os.path.exists(args.db_file): |
79 | 189 | error("Both --file and --db-file must exist when used together") | 189 | logger.error("Both --file and --db-file must exist when used together") |
80 | 190 | if not args.force_merge: | 190 | if not args.force_merge: |
81 | 191 | ans = input("Merge '%s' into '%s' (y|N)? " % (args.file, args.db_file)) | 191 | ans = input("Merge '%s' into '%s' (y|N)? " % (args.file, args.db_file)) |
82 | 192 | if ans.strip().lower() not in ["y", "yes"]: | 192 | if ans.strip().lower() not in ["y", "yes"]: |
84 | 193 | msg("Aborting") | 193 | logger.info("Aborting") |
85 | 194 | sys.exit(0) | 194 | sys.exit(0) |
86 | 195 | 195 | ||
87 | 196 | if args.db_file: # read the existing db | 196 | if args.db_file: # read the existing db |
88 | 197 | try: | 197 | try: |
89 | 198 | db = common.read_file_as_json_dict(args.db_file) | 198 | db = common.read_file_as_json_dict(args.db_file) |
90 | 199 | except json.decoder.JSONDecodeError as e: | 199 | except json.decoder.JSONDecodeError as e: |
92 | 200 | error("Could not read '%s': %s" % (args.db_file, e)) | 200 | logger.error("Could not read '%s': %s" % (args.db_file, e)) |
93 | 201 | 201 | ||
94 | 202 | if args.file and args.db_file: | 202 | if args.file and args.db_file: |
95 | 203 | db = _read_store_dump( | 203 | db = _read_store_dump( |
96 | diff --git a/bin/rock-check-notices b/bin/rock-check-notices | |||
97 | index e2e0271..2b7303c 100755 | |||
98 | --- a/bin/rock-check-notices | |||
99 | +++ b/bin/rock-check-notices | |||
100 | @@ -18,12 +18,11 @@ import argparse | |||
101 | 18 | import json | 18 | import json |
102 | 19 | import os | 19 | import os |
103 | 20 | import sys | 20 | import sys |
104 | 21 | from reviewtools import logger | ||
105 | 21 | 22 | ||
106 | 22 | import reviewtools.common as common | 23 | import reviewtools.common as common |
107 | 23 | from reviewtools.common import ( | 24 | from reviewtools.common import ( |
108 | 24 | cmd, | 25 | cmd, |
109 | 25 | debug, | ||
110 | 26 | warn, | ||
111 | 27 | fetch_usn_db, | 26 | fetch_usn_db, |
112 | 28 | get_debug_info_from_environment, | 27 | get_debug_info_from_environment, |
113 | 29 | initialize_environment_variables, | 28 | initialize_environment_variables, |
114 | @@ -67,10 +66,10 @@ def main(): | |||
115 | 67 | available = "%s/bin/rock-updates-available" % os.path.abspath( | 66 | available = "%s/bin/rock-updates-available" % os.path.abspath( |
116 | 68 | os.environ["SNAP"] | 67 | os.environ["SNAP"] |
117 | 69 | ) | 68 | ) |
119 | 70 | debug("Running " + available) | 69 | logger.debug("Running " + available) |
120 | 71 | else: | 70 | else: |
121 | 72 | available = "review-tools.rock-updates-available" | 71 | available = "review-tools.rock-updates-available" |
123 | 73 | debug("SNAP not set. Defaulting to 'review-tools.*'") | 72 | logger.debug("SNAP not set. Defaulting to 'review-tools.*'") |
124 | 74 | 73 | ||
125 | 75 | reports = dict() | 74 | reports = dict() |
126 | 76 | had_debug = get_debug_info_from_environment() | 75 | had_debug = get_debug_info_from_environment() |
127 | @@ -78,7 +77,7 @@ def main(): | |||
128 | 78 | for pkg in argv: | 77 | for pkg in argv: |
129 | 79 | valid_rock, warn_msg = is_rock_valid(pkg) | 78 | valid_rock, warn_msg = is_rock_valid(pkg) |
130 | 80 | if not valid_rock: | 79 | if not valid_rock: |
132 | 81 | warn(warn_msg) | 80 | logger.warning(warn_msg) |
133 | 82 | continue | 81 | continue |
134 | 83 | 82 | ||
135 | 84 | # The expected rock name for this initial implementation is: | 83 | # The expected rock name for this initial implementation is: |
136 | @@ -89,7 +88,7 @@ def main(): | |||
137 | 89 | rev = os.path.splitext(os.path.basename(pkg))[0].split("_")[1] | 88 | rev = os.path.splitext(os.path.basename(pkg))[0].split("_")[1] |
138 | 90 | 89 | ||
139 | 91 | if rock in reports and rev in reports[rock]: | 90 | if rock in reports and rev in reports[rock]: |
141 | 92 | debug("Skipping %s with revision %s" % (rock, rev)) | 91 | logger.debug("Skipping %s with revision %s" % (rock, rev)) |
142 | 93 | continue | 92 | continue |
143 | 94 | 93 | ||
144 | 95 | usndb_fn = os.path.join(os.environ["SNAP_USER_COMMON"], usndb) | 94 | usndb_fn = os.path.join(os.environ["SNAP_USER_COMMON"], usndb) |
145 | @@ -112,7 +111,7 @@ def main(): | |||
146 | 112 | os.environ["SNAP_DEBUG"] = had_debug | 111 | os.environ["SNAP_DEBUG"] = had_debug |
147 | 113 | 112 | ||
148 | 114 | if rc != 0: | 113 | if rc != 0: |
150 | 115 | warn(out) | 114 | logger.warning(out) |
151 | 116 | continue | 115 | continue |
152 | 117 | 116 | ||
153 | 118 | if rock not in reports: | 117 | if rock not in reports: |
154 | diff --git a/bin/rock-updates-available b/bin/rock-updates-available | |||
155 | index 77a8c42..a6f12bf 100755 | |||
156 | --- a/bin/rock-updates-available | |||
157 | +++ b/bin/rock-updates-available | |||
158 | @@ -44,7 +44,7 @@ import textwrap | |||
159 | 44 | import reviewtools.available as available | 44 | import reviewtools.available as available |
160 | 45 | 45 | ||
161 | 46 | import reviewtools.common as common | 46 | import reviewtools.common as common |
163 | 47 | from reviewtools.common import error | 47 | from reviewtools import logger |
164 | 48 | 48 | ||
165 | 49 | 49 | ||
166 | 50 | def main(): | 50 | def main(): |
167 | @@ -80,13 +80,13 @@ def main(): | |||
168 | 80 | 80 | ||
169 | 81 | # Arg validation | 81 | # Arg validation |
170 | 82 | if args.rock and not args.usn_db: | 82 | if args.rock and not args.usn_db: |
172 | 83 | error("Must specify --usn-db with --rock") | 83 | logger.error("Must specify --usn-db with --rock") |
173 | 84 | elif args.rock and args.store_db: | 84 | elif args.rock and args.store_db: |
175 | 85 | error("Must not specify --store-db with --rock") | 85 | logger.error("Must not specify --store-db with --rock") |
176 | 86 | elif not args.rock and not args.store_db: | 86 | elif not args.rock and not args.store_db: |
178 | 87 | error("Must specify --rock or --store-db") | 87 | logger.error("Must specify --rock or --store-db") |
179 | 88 | elif args.with_cves and not args.rock: | 88 | elif args.with_cves and not args.rock: |
181 | 89 | error("--with-cves should only be used with --rock") | 89 | logger.error("--with-cves should only be used with --rock") |
182 | 90 | 90 | ||
183 | 91 | ignore_pockets = [pocket.strip() for pocket in args.ignore_pockets.split(",")] | 91 | ignore_pockets = [pocket.strip() for pocket in args.ignore_pockets.split(",")] |
184 | 92 | 92 | ||
185 | @@ -97,7 +97,7 @@ def main(): | |||
186 | 97 | args.usn_db, args.rock, args.with_cves, ignore_pockets | 97 | args.usn_db, args.rock, args.with_cves, ignore_pockets |
187 | 98 | ) | 98 | ) |
188 | 99 | except ValueError as e: | 99 | except ValueError as e: |
190 | 100 | error(e) | 100 | logger.error(e) |
191 | 101 | elif args.store_db: | 101 | elif args.store_db: |
192 | 102 | (_, errors) = available.scan_store( | 102 | (_, errors) = available.scan_store( |
193 | 103 | args.usn_db, | 103 | args.usn_db, |
194 | @@ -108,7 +108,7 @@ def main(): | |||
195 | 108 | ignore_pockets, | 108 | ignore_pockets, |
196 | 109 | ) | 109 | ) |
197 | 110 | if len(errors): | 110 | if len(errors): |
199 | 111 | error("Errors encountered when scanning store entries") | 111 | logger.error("Errors encountered when scanning store entries") |
200 | 112 | 112 | ||
201 | 113 | if report != "": | 113 | if report != "": |
202 | 114 | print(report) | 114 | print(report) |
203 | diff --git a/bin/snap-check-notices b/bin/snap-check-notices | |||
204 | index e1b16c2..81f2bc0 100755 | |||
205 | --- a/bin/snap-check-notices | |||
206 | +++ b/bin/snap-check-notices | |||
207 | @@ -19,11 +19,10 @@ import json | |||
208 | 19 | import os | 19 | import os |
209 | 20 | import sys | 20 | import sys |
210 | 21 | 21 | ||
211 | 22 | from reviewtools import logger | ||
212 | 22 | import reviewtools.common as common | 23 | import reviewtools.common as common |
213 | 23 | from reviewtools.common import ( | 24 | from reviewtools.common import ( |
214 | 24 | cmd, | 25 | cmd, |
215 | 25 | debug, | ||
216 | 26 | warn, | ||
217 | 27 | fetch_usn_db, | 26 | fetch_usn_db, |
218 | 28 | initialize_environment_variables, | 27 | initialize_environment_variables, |
219 | 29 | get_debug_info_from_environment, | 28 | get_debug_info_from_environment, |
220 | @@ -66,17 +65,17 @@ def main(): | |||
221 | 66 | available = "%s/bin/snap-updates-available" % os.path.abspath( | 65 | available = "%s/bin/snap-updates-available" % os.path.abspath( |
222 | 67 | os.environ["SNAP"] | 66 | os.environ["SNAP"] |
223 | 68 | ) | 67 | ) |
225 | 69 | debug("Running " + available) | 68 | logger.debug("Running " + available) |
226 | 70 | else: | 69 | else: |
227 | 71 | available = "review-tools.updates-available" | 70 | available = "review-tools.updates-available" |
229 | 72 | debug("SNAP not set. Defaulting to 'review-tools.*'") | 71 | logger.debug("SNAP not set. Defaulting to 'review-tools.*'") |
230 | 73 | 72 | ||
231 | 74 | reports = dict() | 73 | reports = dict() |
232 | 75 | had_debug = get_debug_info_from_environment() | 74 | had_debug = get_debug_info_from_environment() |
233 | 76 | 75 | ||
234 | 77 | for pkg in argv: | 76 | for pkg in argv: |
235 | 78 | if not os.path.isfile(pkg): | 77 | if not os.path.isfile(pkg): |
237 | 79 | warn("Skipping '%s', not a regular file" % pkg) | 78 | logger.warning("Skipping '%s', not a regular file" % pkg) |
238 | 80 | continue | 79 | continue |
239 | 81 | 80 | ||
240 | 82 | snap = os.path.basename(pkg).split("_")[0] | 81 | snap = os.path.basename(pkg).split("_")[0] |
241 | @@ -85,7 +84,7 @@ def main(): | |||
242 | 85 | rev = os.path.splitext(os.path.basename(pkg))[0].split("_")[1] | 84 | rev = os.path.splitext(os.path.basename(pkg))[0].split("_")[1] |
243 | 86 | 85 | ||
244 | 87 | if snap in reports and rev in reports[snap]: | 86 | if snap in reports and rev in reports[snap]: |
246 | 88 | debug("Skipping %s with revision %s" % (snap, rev)) | 87 | logger.debug("Skipping %s with revision %s" % (snap, rev)) |
247 | 89 | continue | 88 | continue |
248 | 90 | 89 | ||
249 | 91 | usndb_fn = os.path.join(os.environ["SNAP_USER_COMMON"], usndb) | 90 | usndb_fn = os.path.join(os.environ["SNAP_USER_COMMON"], usndb) |
250 | @@ -111,7 +110,7 @@ def main(): | |||
251 | 111 | os.environ["SNAP_DEBUG"] = had_debug | 110 | os.environ["SNAP_DEBUG"] = had_debug |
252 | 112 | 111 | ||
253 | 113 | if rc != 0: | 112 | if rc != 0: |
255 | 114 | warn(out) | 113 | logger.warning(out) |
256 | 115 | continue | 114 | continue |
257 | 116 | 115 | ||
258 | 117 | if snap not in reports: | 116 | if snap not in reports: |
259 | diff --git a/bin/snap-review b/bin/snap-review | |||
260 | index 8199929..fa891d9 100755 | |||
261 | --- a/bin/snap-review | |||
262 | +++ b/bin/snap-review | |||
263 | @@ -11,9 +11,9 @@ import tempfile | |||
264 | 11 | import textwrap | 11 | import textwrap |
265 | 12 | import logging | 12 | import logging |
266 | 13 | 13 | ||
267 | 14 | from reviewtools import logger | ||
268 | 14 | from reviewtools.common import ( | 15 | from reviewtools.common import ( |
269 | 15 | MKDTEMP_PREFIX, | 16 | MKDTEMP_PREFIX, |
270 | 16 | error, | ||
271 | 17 | init_override_state_input, | 17 | init_override_state_input, |
272 | 18 | verify_override_state, | 18 | verify_override_state, |
273 | 19 | ) | 19 | ) |
274 | @@ -72,14 +72,8 @@ def main(): | |||
275 | 72 | parser.add_argument("--state-output", default=None, help="store state output blob") | 72 | parser.add_argument("--state-output", default=None, help="store state output blob") |
276 | 73 | args = parser.parse_args() | 73 | args = parser.parse_args() |
277 | 74 | 74 | ||
278 | 75 | error_output_type = "console" | ||
279 | 76 | if args.json or args.sdk: | ||
280 | 77 | error_output_type = "json" | ||
281 | 78 | |||
282 | 79 | if not os.path.exists(args.filename): | 75 | if not os.path.exists(args.filename): |
286 | 80 | error( | 76 | logger.error("file '%s' does not exist." % args.filename) |
284 | 81 | "file '%s' does not exist." % args.filename, output_type=error_output_type | ||
285 | 82 | ) | ||
287 | 83 | 77 | ||
288 | 84 | modules = modules_mod.get_modules() | 78 | modules = modules_mod.get_modules() |
289 | 85 | if not modules: | 79 | if not modules: |
290 | @@ -93,14 +87,14 @@ def main(): | |||
291 | 93 | overrides["snap_decl_plugs"] = json.loads(plugs_file.read()) | 87 | overrides["snap_decl_plugs"] = json.loads(plugs_file.read()) |
292 | 94 | validate_json(overrides["snap_decl_plugs"]) | 88 | validate_json(overrides["snap_decl_plugs"]) |
293 | 95 | except Exception as e: | 89 | except Exception as e: |
295 | 96 | error("Failed to read plugs: %s" % e, output_type=error_output_type) | 90 | logger.error("Failed to read plugs: %s" % e) |
296 | 97 | if args.slots: | 91 | if args.slots: |
297 | 98 | try: | 92 | try: |
298 | 99 | with open(args.slots, "r") as slots_file: | 93 | with open(args.slots, "r") as slots_file: |
299 | 100 | overrides["snap_decl_slots"] = json.loads(slots_file.read()) | 94 | overrides["snap_decl_slots"] = json.loads(slots_file.read()) |
300 | 101 | validate_json(overrides["snap_decl_slots"]) | 95 | validate_json(overrides["snap_decl_slots"]) |
301 | 102 | except Exception as e: | 96 | except Exception as e: |
303 | 103 | error("Failed to read slots: %s" % e, output_type=error_output_type) | 97 | logger.error("Failed to read slots: %s" % e) |
304 | 104 | if args.allow_classic: | 98 | if args.allow_classic: |
305 | 105 | overrides["snap_allow_classic"] = args.allow_classic | 99 | overrides["snap_allow_classic"] = args.allow_classic |
306 | 106 | if args.allow_gadget: | 100 | if args.allow_gadget: |
307 | @@ -112,27 +106,23 @@ def main(): | |||
308 | 112 | 106 | ||
309 | 113 | # --state-input alone is unsupported | 107 | # --state-input alone is unsupported |
310 | 114 | if args.state_input and not args.state_output: | 108 | if args.state_input and not args.state_output: |
312 | 115 | error( | 109 | logger.error( |
313 | 116 | "Must specify --state-output with --state-input", | 110 | "Must specify --state-output with --state-input", |
314 | 117 | output_type=error_output_type, | ||
315 | 118 | ) | 111 | ) |
316 | 119 | 112 | ||
317 | 120 | if args.state_output: | 113 | if args.state_output: |
318 | 121 | if args.state_input: | 114 | if args.state_input: |
319 | 122 | # --state-input --state-output indicates we have previous state | 115 | # --state-input --state-output indicates we have previous state |
320 | 123 | if not os.path.exists(args.state_input): | 116 | if not os.path.exists(args.state_input): |
322 | 124 | error( | 117 | logger.error( |
323 | 125 | "file '%s' does not exist." % args.state_input, | 118 | "file '%s' does not exist." % args.state_input, |
324 | 126 | output_type=error_output_type, | ||
325 | 127 | ) | 119 | ) |
326 | 128 | try: | 120 | try: |
327 | 129 | with open(args.state_input, "rb") as in_f: | 121 | with open(args.state_input, "rb") as in_f: |
328 | 130 | st = in_f.read().decode("utf-8") | 122 | st = in_f.read().decode("utf-8") |
329 | 131 | overrides["state_input"] = json.loads(st) | 123 | overrides["state_input"] = json.loads(st) |
330 | 132 | except Exception as e: | 124 | except Exception as e: |
334 | 133 | error( | 125 | logger.error("Could not read state input: %s" % e) |
332 | 134 | "Could not read state input: %s" % e, output_type=error_output_type | ||
333 | 135 | ) | ||
335 | 136 | else: | 126 | else: |
336 | 137 | # --state-output alone indicates we are bootstrapping | 127 | # --state-output alone indicates we are bootstrapping |
337 | 138 | overrides["state_input"] = init_override_state_input() | 128 | overrides["state_input"] = init_override_state_input() |
338 | @@ -141,7 +131,7 @@ def main(): | |||
339 | 141 | try: | 131 | try: |
340 | 142 | verify_override_state(overrides["state_input"]) | 132 | verify_override_state(overrides["state_input"]) |
341 | 143 | except Exception as e: | 133 | except Exception as e: |
343 | 144 | error("state-input failed to verify: %s" % e, output_type=error_output_type) | 134 | logger.error("state-input failed to verify: %s" % e) |
344 | 145 | 135 | ||
345 | 146 | overrides["state_output"] = copy.deepcopy(overrides["state_input"]) | 136 | overrides["state_output"] = copy.deepcopy(overrides["state_input"]) |
346 | 147 | 137 | ||
347 | diff --git a/bin/snap-updates-available b/bin/snap-updates-available | |||
348 | index 4db68f9..98e888a 100755 | |||
349 | --- a/bin/snap-updates-available | |||
350 | +++ b/bin/snap-updates-available | |||
351 | @@ -45,7 +45,7 @@ import textwrap | |||
352 | 45 | import reviewtools.available as available | 45 | import reviewtools.available as available |
353 | 46 | 46 | ||
354 | 47 | import reviewtools.common as common | 47 | import reviewtools.common as common |
356 | 48 | from reviewtools.common import error | 48 | from reviewtools import logger |
357 | 49 | 49 | ||
358 | 50 | 50 | ||
359 | 51 | def main(): | 51 | def main(): |
360 | @@ -86,19 +86,19 @@ def main(): | |||
361 | 86 | 86 | ||
362 | 87 | # Arg validation | 87 | # Arg validation |
363 | 88 | if args.check_shared_publishers and args.snap: | 88 | if args.check_shared_publishers and args.snap: |
365 | 89 | error("--snap should not be used with --check-shared-publishers") | 89 | logger.error("--snap should not be used with --check-shared-publishers") |
366 | 90 | elif args.check_shared_publishers and args.usn_db: | 90 | elif args.check_shared_publishers and args.usn_db: |
368 | 91 | error("--usn-db should not be used with --check-shared-publishers") | 91 | logger.error("--usn-db should not be used with --check-shared-publishers") |
369 | 92 | elif args.check_shared_publishers and not args.store_db: | 92 | elif args.check_shared_publishers and not args.store_db: |
371 | 93 | error("Must specify --store-db with --check-shared-publishers") | 93 | logger.error("Must specify --store-db with --check-shared-publishers") |
372 | 94 | elif args.snap and not args.usn_db: | 94 | elif args.snap and not args.usn_db: |
374 | 95 | error("Must specify --usn-db with --snap") | 95 | logger.error("Must specify --usn-db with --snap") |
375 | 96 | elif args.snap and args.store_db: | 96 | elif args.snap and args.store_db: |
377 | 97 | error("Must not specify --store-db with --snap") | 97 | logger.error("Must not specify --store-db with --snap") |
378 | 98 | elif not args.snap and not args.store_db: | 98 | elif not args.snap and not args.store_db: |
380 | 99 | error("Must specify --snap or --store-db") | 99 | logger.error("Must specify --snap or --store-db") |
381 | 100 | elif args.with_cves and not args.snap: | 100 | elif args.with_cves and not args.snap: |
383 | 101 | error("--with-cves should only be used with --snap") | 101 | logger.error("--with-cves should only be used with --snap") |
384 | 102 | 102 | ||
385 | 103 | ignore_pockets = [pocket.strip() for pocket in args.ignore_pockets.split(",")] | 103 | ignore_pockets = [pocket.strip() for pocket in args.ignore_pockets.split(",")] |
386 | 104 | 104 | ||
387 | @@ -111,7 +111,7 @@ def main(): | |||
388 | 111 | args.usn_db, args.snap, args.with_cves, ignore_pockets | 111 | args.usn_db, args.snap, args.with_cves, ignore_pockets |
389 | 112 | ) | 112 | ) |
390 | 113 | except ValueError as e: | 113 | except ValueError as e: |
392 | 114 | error(e) | 114 | logger.error(e) |
393 | 115 | elif args.store_db: | 115 | elif args.store_db: |
394 | 116 | (_, errors) = available.scan_store( | 116 | (_, errors) = available.scan_store( |
395 | 117 | args.usn_db, | 117 | args.usn_db, |
396 | @@ -122,7 +122,7 @@ def main(): | |||
397 | 122 | ignore_pockets, | 122 | ignore_pockets, |
398 | 123 | ) | 123 | ) |
399 | 124 | if len(errors): | 124 | if len(errors): |
401 | 125 | error("Errors encountered when scanning store entries") | 125 | logger.error("Errors encountered when scanning store entries") |
402 | 126 | 126 | ||
403 | 127 | if report != "": | 127 | if report != "": |
404 | 128 | print(report) | 128 | print(report) |
405 | diff --git a/bin/snap-verify-declaration b/bin/snap-verify-declaration | |||
406 | index 7d7ee40..cb97a90 100755 | |||
407 | --- a/bin/snap-verify-declaration | |||
408 | +++ b/bin/snap-verify-declaration | |||
409 | @@ -22,7 +22,7 @@ import json | |||
410 | 22 | import sys | 22 | import sys |
411 | 23 | import textwrap | 23 | import textwrap |
412 | 24 | 24 | ||
414 | 25 | from reviewtools.common import error | 25 | from reviewtools import logger |
415 | 26 | import reviewtools.sr_declaration as sr_declaration | 26 | import reviewtools.sr_declaration as sr_declaration |
416 | 27 | 27 | ||
417 | 28 | 28 | ||
418 | @@ -50,12 +50,12 @@ def main(): | |||
419 | 50 | ) | 50 | ) |
420 | 51 | args = parser.parse_args() | 51 | args = parser.parse_args() |
421 | 52 | 52 | ||
423 | 53 | error_output_type = "console" | 53 | output_type = "console" |
424 | 54 | if args.json: | 54 | if args.json: |
426 | 55 | error_output_type = "json" | 55 | output_type = "json" |
427 | 56 | 56 | ||
428 | 57 | if not args.plugs and not args.slots: | 57 | if not args.plugs and not args.slots: |
430 | 58 | error("Must specify --plugs and/or --slots", output_type=error_output_type) | 58 | logger.error("Must specify --plugs and/or --slots") |
431 | 59 | 59 | ||
432 | 60 | # Read in the snap declaration | 60 | # Read in the snap declaration |
433 | 61 | snap_decl = {"plugs": {}, "slots": {}} | 61 | snap_decl = {"plugs": {}, "slots": {}} |
434 | @@ -65,25 +65,22 @@ def main(): | |||
435 | 65 | snap_decl["plugs"] = json.loads(plugs_file.read()) | 65 | snap_decl["plugs"] = json.loads(plugs_file.read()) |
436 | 66 | sr_declaration.validate_json(snap_decl["plugs"]) | 66 | sr_declaration.validate_json(snap_decl["plugs"]) |
437 | 67 | except Exception as e: | 67 | except Exception as e: |
439 | 68 | error("Failed to read plugs: %s" % e, output_type=error_output_type) | 68 | logger.error("Failed to read plugs: %s" % e) |
440 | 69 | if args.slots: | 69 | if args.slots: |
441 | 70 | try: | 70 | try: |
442 | 71 | with open(args.slots, "r") as slots_file: | 71 | with open(args.slots, "r") as slots_file: |
443 | 72 | snap_decl["slots"] = json.loads(slots_file.read()) | 72 | snap_decl["slots"] = json.loads(slots_file.read()) |
444 | 73 | sr_declaration.validate_json(snap_decl["slots"]) | 73 | sr_declaration.validate_json(snap_decl["slots"]) |
445 | 74 | except Exception as e: | 74 | except Exception as e: |
447 | 75 | error("Failed to read slots: %s" % e, output_type=error_output_type) | 75 | logger.error("Failed to read slots: %s" % e) |
448 | 76 | 76 | ||
449 | 77 | try: | 77 | try: |
450 | 78 | review = sr_declaration.verify_snap_declaration(snap_decl) | 78 | review = sr_declaration.verify_snap_declaration(snap_decl) |
451 | 79 | except Exception as e: | 79 | except Exception as e: |
456 | 80 | error( | 80 | logger.error("verify_snap_declaration() raised exception for snap decl: %s" % e) |
453 | 81 | "verify_snap_declaration() raised exception for snap decl: %s" % e, | ||
454 | 82 | output_type=error_output_type, | ||
455 | 83 | ) | ||
457 | 84 | 81 | ||
458 | 85 | report = review.review_report | 82 | report = review.review_report |
460 | 86 | report.show_summary(error_output_type) | 83 | report.show_summary(output_type) |
461 | 87 | 84 | ||
462 | 88 | rc = 0 | 85 | rc = 0 |
463 | 89 | if report.errors: | 86 | if report.errors: |
464 | diff --git a/bin/store-query b/bin/store-query | |||
465 | index e6b22b8..420e81f 100755 | |||
466 | --- a/bin/store-query | |||
467 | +++ b/bin/store-query | |||
468 | @@ -35,7 +35,7 @@ import requests | |||
469 | 35 | import yaml | 35 | import yaml |
470 | 36 | 36 | ||
471 | 37 | import reviewtools.common as common | 37 | import reviewtools.common as common |
473 | 38 | from reviewtools.common import debug, error | 38 | from reviewtools import logger |
474 | 39 | 39 | ||
475 | 40 | from reviewtools.sr_common import SnapReview | 40 | from reviewtools.sr_common import SnapReview |
476 | 41 | 41 | ||
477 | @@ -140,10 +140,10 @@ def _fetch(url, headers=None): | |||
478 | 140 | 140 | ||
479 | 141 | h = str(headers) | 141 | h = str(headers) |
480 | 142 | if url in responses and h in responses[url]: | 142 | if url in responses and h in responses[url]: |
482 | 143 | debug("Cache: %s, headers=%s" % (url, headers)) | 143 | logger.debug("Cache: %s, headers=%s" % (url, headers)) |
483 | 144 | return responses[url][h] | 144 | return responses[url][h] |
484 | 145 | 145 | ||
486 | 146 | debug("Fetch: %s, headers=%s" % (url, headers)) | 146 | logger.debug("Fetch: %s, headers=%s" % (url, headers)) |
487 | 147 | if headers is None: | 147 | if headers is None: |
488 | 148 | r = requests.get(url) | 148 | r = requests.get(url) |
489 | 149 | else: | 149 | else: |
490 | @@ -152,7 +152,7 @@ def _fetch(url, headers=None): | |||
491 | 152 | if r.status_code != 200: | 152 | if r.status_code != 200: |
492 | 153 | raise Exception("Status code: %s (%s)" % (r.status_code, r.headers)) | 153 | raise Exception("Status code: %s (%s)" % (r.status_code, r.headers)) |
493 | 154 | 154 | ||
495 | 155 | debug("Response headers=%s" % r.headers) | 155 | logger.debug("Response headers=%s" % r.headers) |
496 | 156 | 156 | ||
497 | 157 | if url not in responses: | 157 | if url not in responses: |
498 | 158 | responses[url] = {} | 158 | responses[url] = {} |
499 | @@ -264,7 +264,7 @@ def _get_snap_yaml(snap_name, channel=None, arch=None): | |||
500 | 264 | ): | 264 | ): |
501 | 265 | return yaml.load(item["snap-yaml"], Loader=yaml.SafeLoader) | 265 | return yaml.load(item["snap-yaml"], Loader=yaml.SafeLoader) |
502 | 266 | 266 | ||
504 | 267 | debug(format_resp(res, output_json=True)) | 267 | logger.debug(format_resp(res, output_json=True)) |
505 | 268 | raise Exception( | 268 | raise Exception( |
506 | 269 | "Could not find snap-yaml for (channel=%s, arch=%s)" % (channel, arch) | 269 | "Could not find snap-yaml for (channel=%s, arch=%s)" % (channel, arch) |
507 | 270 | ) | 270 | ) |
508 | @@ -360,10 +360,12 @@ def main(): | |||
509 | 360 | args = parser.parse_args() | 360 | args = parser.parse_args() |
510 | 361 | 361 | ||
511 | 362 | if args.name_or_id is None and args.snap_id is None: | 362 | if args.name_or_id is None and args.snap_id is None: |
513 | 363 | error("Must specify a snap name or snap id. Aborting") | 363 | logger.error("Must specify a snap name or snap id. Aborting") |
514 | 364 | 364 | ||
515 | 365 | if not args.snap_decl and not args.snap_info and not args.snap_yaml: | 365 | if not args.snap_decl and not args.snap_info and not args.snap_yaml: |
517 | 366 | error("Must specify one of --snap-decl, --snap-info or --snap-yaml. Aborting") | 366 | logger.error( |
518 | 367 | "Must specify one of --snap-decl, --snap-info or --snap-yaml. Aborting" | ||
519 | 368 | ) | ||
520 | 367 | 369 | ||
521 | 368 | # Note snap names are lower case, with numbers and '-' but snap ids are | 370 | # Note snap names are lower case, with numbers and '-' but snap ids are |
522 | 369 | # alphanumerics of exactly 32. While possible that a snap-id and a name | 371 | # alphanumerics of exactly 32. While possible that a snap-id and a name |
523 | @@ -398,7 +400,7 @@ def main(): | |||
524 | 398 | if mf < 0: | 400 | if mf < 0: |
525 | 399 | raise ValueError | 401 | raise ValueError |
526 | 400 | except ValueError: | 402 | except ValueError: |
528 | 401 | error("--max-format should be 'all' or integer >= 0") | 403 | logger.error("--max-format should be 'all' or integer >= 0") |
529 | 402 | 404 | ||
530 | 403 | if args.max_format == "all": | 405 | if args.max_format == "all": |
531 | 404 | snap_decl = _get_all_snap_declarations_by_id(snap_id) | 406 | snap_decl = _get_all_snap_declarations_by_id(snap_id) |
532 | @@ -416,4 +418,4 @@ if __name__ == "__main__": | |||
533 | 416 | try: | 418 | try: |
534 | 417 | main() | 419 | main() |
535 | 418 | except KeyboardInterrupt: | 420 | except KeyboardInterrupt: |
537 | 419 | error("Aborted.") | 421 | logger.error("Aborted.") |
538 | diff --git a/reviewtools/__init__.py b/reviewtools/__init__.py | |||
539 | index e69de29..4ce6539 100644 | |||
540 | --- a/reviewtools/__init__.py | |||
541 | +++ b/reviewtools/__init__.py | |||
542 | @@ -0,0 +1,16 @@ | |||
543 | 1 | import logging | ||
544 | 2 | |||
545 | 3 | |||
546 | 4 | class ShutdownHandler(logging.StreamHandler): | ||
547 | 5 | def emit(self, record): | ||
548 | 6 | super().emit(record) | ||
549 | 7 | if record.levelno >= logging.ERROR: | ||
550 | 8 | exit(1) | ||
551 | 9 | |||
552 | 10 | |||
553 | 11 | logger = logging.getLogger(__name__) | ||
554 | 12 | |||
555 | 13 | handler = ShutdownHandler() | ||
556 | 14 | myFormatter = logging.Formatter("%(levelname)s: %(message)s") | ||
557 | 15 | handler.setFormatter(myFormatter) | ||
558 | 16 | logger.addHandler(handler) | ||
559 | diff --git a/reviewtools/available.py b/reviewtools/available.py | |||
560 | index 5b0dd3c..94bfbe1 100755 | |||
561 | --- a/reviewtools/available.py | |||
562 | +++ b/reviewtools/available.py | |||
563 | @@ -18,10 +18,8 @@ import os | |||
564 | 18 | import shutil | 18 | import shutil |
565 | 19 | import tempfile | 19 | import tempfile |
566 | 20 | 20 | ||
567 | 21 | from reviewtools import logger | ||
568 | 21 | from reviewtools.common import ( | 22 | from reviewtools.common import ( |
569 | 22 | error, | ||
570 | 23 | debug, | ||
571 | 24 | warn, | ||
572 | 25 | open_file_write, | 23 | open_file_write, |
573 | 26 | read_file_as_json_dict, | 24 | read_file_as_json_dict, |
574 | 27 | MKDTEMP_PREFIX, | 25 | MKDTEMP_PREFIX, |
575 | @@ -356,7 +354,7 @@ def _email_report_for_pkg(pkg_db, seen_db): | |||
576 | 356 | 354 | ||
577 | 357 | email.send(email_to_addr, subj, body, bcc) | 355 | email.send(email_to_addr, subj, body, bcc) |
578 | 358 | 356 | ||
580 | 359 | debug("Sent email for '%s'" % pkgname) | 357 | logger.debug("Sent email for '%s'" % pkgname) |
581 | 360 | return (email_to_addr, subj, body) | 358 | return (email_to_addr, subj, body) |
582 | 361 | 359 | ||
583 | 362 | 360 | ||
584 | @@ -469,7 +467,7 @@ def scan_store( | |||
585 | 469 | continue | 467 | continue |
586 | 470 | 468 | ||
587 | 471 | if body is None: # pragma: nocover | 469 | if body is None: # pragma: nocover |
589 | 472 | debug("Skipped email for '%s': up to date" % pkg_db["name"]) | 470 | logger.debug("Skipped email for '%s': up to date" % pkg_db["name"]) |
590 | 473 | 471 | ||
591 | 474 | if seen_db_fn: | 472 | if seen_db_fn: |
592 | 475 | _update_seen(seen_db_fn, seen_db, pkg_db) | 473 | _update_seen(seen_db_fn, seen_db, pkg_db) |
593 | @@ -477,7 +475,7 @@ def scan_store( | |||
594 | 477 | if len(errors) > 0: | 475 | if len(errors) > 0: |
595 | 478 | for p in errors: | 476 | for p in errors: |
596 | 479 | for e in errors[p]: | 477 | for e in errors[p]: |
598 | 480 | warn("%s: %s" % (p, e)) | 478 | logger.warning("%s: %s" % (p, e)) |
599 | 481 | 479 | ||
600 | 482 | return sent, errors | 480 | return sent, errors |
601 | 483 | 481 | ||
602 | @@ -489,7 +487,7 @@ def scan_snap(secnot_db_fn, snap_fn, with_cves=False, ignore_pockets=list()): | |||
603 | 489 | snap = SnapContainer(snap_fn) | 487 | snap = SnapContainer(snap_fn) |
604 | 490 | (man, dpkg) = snap.get_snap_manifest() | 488 | (man, dpkg) = snap.get_snap_manifest() |
605 | 491 | except ContainerException as e: | 489 | except ContainerException as e: |
607 | 492 | error(str(e)) | 490 | logger.error(str(e)) |
608 | 493 | man = get_faked_build_and_stage_packages(man) | 491 | man = get_faked_build_and_stage_packages(man) |
609 | 494 | 492 | ||
610 | 495 | # Use dpkg.list when the snap ships it in a spot we honor. This is limited | 493 | # Use dpkg.list when the snap ships it in a spot we honor. This is limited |
611 | diff --git a/reviewtools/common.py b/reviewtools/common.py | |||
612 | index eaaadee..2b8472e 100644 | |||
613 | --- a/reviewtools/common.py | |||
614 | +++ b/reviewtools/common.py | |||
615 | @@ -20,6 +20,7 @@ import copy | |||
616 | 20 | from enum import Enum | 20 | from enum import Enum |
617 | 21 | import inspect | 21 | import inspect |
618 | 22 | import json | 22 | import json |
619 | 23 | from reviewtools import logger | ||
620 | 23 | import os | 24 | import os |
621 | 24 | from pkg_resources import resource_filename | 25 | from pkg_resources import resource_filename |
622 | 25 | import re | 26 | import re |
623 | @@ -229,51 +230,9 @@ class ReviewBase(object): | |||
624 | 229 | # | 230 | # |
625 | 230 | 231 | ||
626 | 231 | 232 | ||
627 | 232 | def error(out, exit_code=1, do_exit=True, output_type=None): | ||
628 | 233 | """Print error message and exit""" | ||
629 | 234 | global REPORT_OUTPUT | ||
630 | 235 | if output_type is None: | ||
631 | 236 | output_type = REPORT_OUTPUT | ||
632 | 237 | |||
633 | 238 | if output_type == "json": | ||
634 | 239 | report = Report() | ||
635 | 240 | report.add_result("runtime-errors", "error", "msg", out, manual_review=True) | ||
636 | 241 | report.show_results(output_type) | ||
637 | 242 | else: | ||
638 | 243 | print("ERROR: %s" % (out), file=sys.stderr) | ||
639 | 244 | |||
640 | 245 | if do_exit: | ||
641 | 246 | sys.exit(exit_code) | ||
642 | 247 | |||
643 | 248 | |||
644 | 249 | def warn(out): | ||
645 | 250 | """Print warning message""" | ||
646 | 251 | try: | ||
647 | 252 | print("WARN: %s" % (out), file=sys.stderr) | ||
648 | 253 | except IOError: | ||
649 | 254 | pass | ||
650 | 255 | |||
651 | 256 | |||
652 | 257 | def msg(out, output=sys.stdout): | ||
653 | 258 | """Print message""" | ||
654 | 259 | try: | ||
655 | 260 | print("%s" % (out), file=output) | ||
656 | 261 | except IOError: | ||
657 | 262 | pass | ||
658 | 263 | |||
659 | 264 | |||
660 | 265 | def debug(out): | ||
661 | 266 | """Print debug message""" | ||
662 | 267 | if "SNAP_DEBUG" in os.environ: | ||
663 | 268 | try: | ||
664 | 269 | print("DEBUG: %s" % (out), file=sys.stderr) | ||
665 | 270 | except IOError: | ||
666 | 271 | pass | ||
667 | 272 | |||
668 | 273 | |||
669 | 274 | def cmd(command, stdout=subprocess.PIPE, stderr=subprocess.STDOUT): | 233 | def cmd(command, stdout=subprocess.PIPE, stderr=subprocess.STDOUT): |
670 | 275 | """Try to execute the given command.""" | 234 | """Try to execute the given command.""" |
672 | 276 | debug(" ".join(command)) | 235 | logger.debug(" ".join(command)) |
673 | 277 | try: | 236 | try: |
674 | 278 | sp = subprocess.Popen(command, stdout=stdout, stderr=stderr) | 237 | sp = subprocess.Popen(command, stdout=stdout, stderr=stderr) |
675 | 279 | except OSError as ex: | 238 | except OSError as ex: |
676 | @@ -595,14 +554,14 @@ def _unpack_rock_tar(rock_pkg, dest): | |||
677 | 595 | # https://docs.python.org/3/library/tarfile.html#tarfile.TarFile.extractall | 554 | # https://docs.python.org/3/library/tarfile.html#tarfile.TarFile.extractall |
678 | 596 | for name in tar.getnames(): | 555 | for name in tar.getnames(): |
679 | 597 | if name.startswith("..") or name.startswith("/"): | 556 | if name.startswith("..") or name.startswith("/"): |
681 | 598 | error( | 557 | logger.error( |
682 | 599 | "Bad path %s while extracting archive at %s" | 558 | "Bad path %s while extracting archive at %s" |
683 | 600 | % (name, rock_pkg) | 559 | % (name, rock_pkg) |
684 | 601 | ) | 560 | ) |
685 | 602 | 561 | ||
686 | 603 | tar.extractall(path=d) | 562 | tar.extractall(path=d) |
687 | 604 | except Exception as e: | 563 | except Exception as e: |
689 | 605 | error("Unexpected exception while unpacking rock %s" % e) | 564 | logger.error("Unexpected exception while unpacking rock %s" % e) |
690 | 606 | if os.path.isdir(d): | 565 | if os.path.isdir(d): |
691 | 607 | recursive_rm(d) | 566 | recursive_rm(d) |
692 | 608 | 567 | ||
693 | @@ -615,17 +574,17 @@ def _unpack_rock_tar(rock_pkg, dest): | |||
694 | 615 | 574 | ||
695 | 616 | return dest | 575 | return dest |
696 | 617 | else: | 576 | else: |
698 | 618 | error(error_msg) | 577 | logger.error(error_msg) |
699 | 619 | 578 | ||
700 | 620 | 579 | ||
701 | 621 | def check_dir(dest): | 580 | def check_dir(dest): |
702 | 622 | if dest is not None and os.path.exists(dest): | 581 | if dest is not None and os.path.exists(dest): |
704 | 623 | error("'%s' exists. Aborting." % dest) | 582 | logger.error("'%s' exists. Aborting." % dest) |
705 | 624 | 583 | ||
706 | 625 | 584 | ||
707 | 626 | def check_fn(fn): | 585 | def check_fn(fn): |
708 | 627 | if not os.path.isfile(fn): | 586 | if not os.path.isfile(fn): |
710 | 628 | error("Could not find '%s'" % fn) | 587 | logger.error("Could not find '%s'" % fn) |
711 | 629 | pkg = fn | 588 | pkg = fn |
712 | 630 | if not pkg.startswith("/"): | 589 | if not pkg.startswith("/"): |
713 | 631 | pkg = os.path.abspath(pkg) | 590 | pkg = os.path.abspath(pkg) |
714 | @@ -636,7 +595,7 @@ def check_max_pkg_size(pkg): | |||
715 | 636 | size = os.stat(pkg)[stat.ST_SIZE] | 595 | size = os.stat(pkg)[stat.ST_SIZE] |
716 | 637 | max = MAX_COMPRESSED_SIZE * 1024 * 1024 * 1024 | 596 | max = MAX_COMPRESSED_SIZE * 1024 * 1024 * 1024 |
717 | 638 | if size > max: | 597 | if size > max: |
719 | 639 | error( | 598 | logger.error( |
720 | 640 | "compressed file is too large (%dM > %dM)" | 599 | "compressed file is too large (%dM > %dM)" |
721 | 641 | % (size / 1024 / 1024, max / 1024 / 1024) | 600 | % (size / 1024 / 1024, max / 1024 / 1024) |
722 | 642 | ) | 601 | ) |
723 | @@ -654,7 +613,7 @@ def unpack_rock(fn, dest=None): | |||
724 | 654 | if tarfile.is_tarfile(pkg): | 613 | if tarfile.is_tarfile(pkg): |
725 | 655 | return _unpack_rock_tar(fn, dest) | 614 | return _unpack_rock_tar(fn, dest) |
726 | 656 | 615 | ||
728 | 657 | error("Unsupported package format (not tar)") | 616 | logger.error("Unsupported package format (not tar)") |
729 | 658 | 617 | ||
730 | 659 | 618 | ||
731 | 660 | def open_file_read(path): | 619 | def open_file_read(path): |
732 | @@ -895,7 +854,7 @@ def check_results( | |||
733 | 895 | def read_file_as_json_dict(fn): | 854 | def read_file_as_json_dict(fn): |
734 | 896 | """Read in filename as json dict""" | 855 | """Read in filename as json dict""" |
735 | 897 | # XXX: consider reading in as stream | 856 | # XXX: consider reading in as stream |
737 | 898 | debug("Loading: %s" % fn) | 857 | logger.debug("Loading: %s" % fn) |
738 | 899 | raw = {} | 858 | raw = {} |
739 | 900 | with open_file_read(fn) as fd: | 859 | with open_file_read(fn) as fd: |
740 | 901 | try: | 860 | try: |
741 | @@ -965,7 +924,7 @@ def build_manifest_from_rock_tar(man_fn, unpack_tmp_dir): | |||
742 | 965 | 924 | ||
743 | 966 | if not dpkg_query: | 925 | if not dpkg_query: |
744 | 967 | recursive_rm(unpack_tmp_dir) | 926 | recursive_rm(unpack_tmp_dir) |
746 | 968 | error("%s not in %s" % (man_fn, unpack_tmp_dir)) | 927 | logger.error("%s not in %s" % (man_fn, unpack_tmp_dir)) |
747 | 969 | 928 | ||
748 | 970 | return build_man_from_dpkg_query_file_content(dpkg_query) | 929 | return build_man_from_dpkg_query_file_content(dpkg_query) |
749 | 971 | 930 | ||
750 | @@ -1037,7 +996,7 @@ def extract_dpkg_query_file_from_rock(dpkg_query, unpack_tmp_dir): | |||
751 | 1037 | if dpkg_query: | 996 | if dpkg_query: |
752 | 1038 | return dpkg_query | 997 | return dpkg_query |
753 | 1039 | except Exception as e: | 998 | except Exception as e: |
755 | 1040 | error("Could not extract manifest %s" % e) | 999 | logger.error("Could not extract manifest %s" % e) |
756 | 1041 | recursive_rm(unpack_tmp_dir) | 1000 | recursive_rm(unpack_tmp_dir) |
757 | 1042 | 1001 | ||
758 | 1043 | return dpkg_query | 1002 | return dpkg_query |
759 | @@ -1061,7 +1020,7 @@ def read_snapd_base_declaration(): | |||
760 | 1061 | if not os.path.exists(bd_fn): | 1020 | if not os.path.exists(bd_fn): |
761 | 1062 | bd_fn = resource_filename(__name__, "data/snapd-base-declaration.yaml") | 1021 | bd_fn = resource_filename(__name__, "data/snapd-base-declaration.yaml") |
762 | 1063 | if not os.path.exists(bd_fn): | 1022 | if not os.path.exists(bd_fn): |
764 | 1064 | error("could not find '%s'" % bd_fn) | 1023 | logger.error("could not find '%s'" % bd_fn) |
765 | 1065 | fd = open_file_read(bd_fn) | 1024 | fd = open_file_read(bd_fn) |
766 | 1066 | contents = fd.read() | 1025 | contents = fd.read() |
767 | 1067 | fd.close() | 1026 | fd.close() |
768 | @@ -1128,12 +1087,12 @@ def initialize_environment_variables(): | |||
769 | 1128 | os.environ["SNAP_USER_COMMON"] = ( | 1087 | os.environ["SNAP_USER_COMMON"] = ( |
770 | 1129 | "%s/snap/review-tools/common" % os.environ["HOME"] | 1088 | "%s/snap/review-tools/common" % os.environ["HOME"] |
771 | 1130 | ) | 1089 | ) |
773 | 1131 | debug( | 1090 | logger.debug( |
774 | 1132 | "SNAP_USER_COMMON not set. Defaulting to %s" | 1091 | "SNAP_USER_COMMON not set. Defaulting to %s" |
775 | 1133 | % os.environ["SNAP_USER_COMMON"] | 1092 | % os.environ["SNAP_USER_COMMON"] |
776 | 1134 | ) | 1093 | ) |
777 | 1135 | if not os.path.exists(os.environ["SNAP_USER_COMMON"]): | 1094 | if not os.path.exists(os.environ["SNAP_USER_COMMON"]): |
779 | 1136 | error( | 1095 | logger.error( |
780 | 1137 | "SNAP_USER_COMMON not set and %s does not exist" | 1096 | "SNAP_USER_COMMON not set and %s does not exist" |
781 | 1138 | % os.environ["SNAP_USER_COMMON"] | 1097 | % os.environ["SNAP_USER_COMMON"] |
782 | 1139 | ) | 1098 | ) |
783 | @@ -1162,9 +1121,9 @@ def fetch_usn_db(args): | |||
784 | 1162 | ): | 1121 | ): |
785 | 1163 | rc, out = cmd([fetchdb, "%s.bz2" % usndb]) | 1122 | rc, out = cmd([fetchdb, "%s.bz2" % usndb]) |
786 | 1164 | if rc != 0: | 1123 | if rc != 0: |
788 | 1165 | error(out) | 1124 | logger.error(out) |
789 | 1166 | else: | 1125 | else: |
791 | 1167 | debug("Reusing %s" % usndb) | 1126 | logger.debug("Reusing %s" % usndb) |
792 | 1168 | os.chdir(curdir) | 1127 | os.chdir(curdir) |
793 | 1169 | return usndb | 1128 | return usndb |
794 | 1170 | 1129 | ||
795 | diff --git a/reviewtools/sr_declaration.py b/reviewtools/sr_declaration.py | |||
796 | index edfe023..e4df23b 100644 | |||
797 | --- a/reviewtools/sr_declaration.py | |||
798 | +++ b/reviewtools/sr_declaration.py | |||
799 | @@ -15,8 +15,9 @@ | |||
800 | 15 | # along with this program. If not, see <http://www.gnu.org/licenses/>. | 15 | # along with this program. If not, see <http://www.gnu.org/licenses/>. |
801 | 16 | 16 | ||
802 | 17 | from __future__ import print_function | 17 | from __future__ import print_function |
803 | 18 | from reviewtools import logger | ||
804 | 18 | from reviewtools.sr_common import SnapReview, SnapReviewException | 19 | from reviewtools.sr_common import SnapReview, SnapReviewException |
806 | 19 | from reviewtools.common import error, ReviewBase, read_snapd_base_declaration | 20 | from reviewtools.common import ReviewBase, read_snapd_base_declaration |
807 | 20 | from reviewtools.overrides import sec_iface_ref_overrides | 21 | from reviewtools.overrides import sec_iface_ref_overrides |
808 | 21 | from reviewtools.schemas.schema_validator import load_schema | 22 | from reviewtools.schemas.schema_validator import load_schema |
809 | 22 | import copy | 23 | import copy |
810 | @@ -1521,7 +1522,7 @@ def verify_snap_declaration(snap_decl, base_decl=None): | |||
811 | 1521 | try: | 1522 | try: |
812 | 1522 | SnapReviewDeclaration._verify_declaration(review, base_decl, base=True) | 1523 | SnapReviewDeclaration._verify_declaration(review, base_decl, base=True) |
813 | 1523 | except Exception as e: # pragma: nocover | 1524 | except Exception as e: # pragma: nocover |
815 | 1524 | error("_verify_declaration() raised exception for base decl: %s" % e) | 1525 | logger.error("_verify_declaration() raised exception for base decl: %s" % e) |
816 | 1525 | 1526 | ||
817 | 1526 | # First make sure that the interfaces in the snap declaration are known to | 1527 | # First make sure that the interfaces in the snap declaration are known to |
818 | 1527 | # the base declaration | 1528 | # the base declaration |
819 | @@ -1543,6 +1544,6 @@ def verify_snap_declaration(snap_decl, base_decl=None): | |||
820 | 1543 | try: | 1544 | try: |
821 | 1544 | SnapReviewDeclaration._verify_declaration(review, snap_decl, base=False) | 1545 | SnapReviewDeclaration._verify_declaration(review, snap_decl, base=False) |
822 | 1545 | except Exception as e: # pragma: nocover | 1546 | except Exception as e: # pragma: nocover |
824 | 1546 | error("_verify_declaration() raised exception for snap decl: %s" % e) | 1547 | logger.error("_verify_declaration() raised exception for snap decl: %s" % e) |
825 | 1547 | 1548 | ||
826 | 1548 | return review | 1549 | return review |
827 | diff --git a/reviewtools/store.py b/reviewtools/store.py | |||
828 | index e84cd00..2d0a3ac 100644 | |||
829 | --- a/reviewtools/store.py | |||
830 | +++ b/reviewtools/store.py | |||
831 | @@ -19,9 +19,8 @@ import yaml | |||
832 | 19 | 19 | ||
833 | 20 | import reviewtools.debversion as debversion | 20 | import reviewtools.debversion as debversion |
834 | 21 | 21 | ||
835 | 22 | from reviewtools import logger | ||
836 | 22 | from reviewtools.common import ( | 23 | from reviewtools.common import ( |
837 | 23 | debug, | ||
838 | 24 | warn, | ||
839 | 25 | get_os_codename, | 24 | get_os_codename, |
840 | 26 | assign_type_to_dict_values, | 25 | assign_type_to_dict_values, |
841 | 27 | _add_error, # make a class | 26 | _add_error, # make a class |
842 | @@ -233,7 +232,7 @@ def get_pkg_revisions(item, secnot_db, errors, pkg_type="snap", ignore_pockets=l | |||
843 | 233 | continue | 232 | continue |
844 | 234 | 233 | ||
845 | 235 | r = str(rev["revision"]) # ensure yaml and json agree on type | 234 | r = str(rev["revision"]) # ensure yaml and json agree on type |
847 | 236 | debug("Checking %s r%s" % (item["name"], r)) | 235 | logger.debug("Checking %s r%s" % (item["name"], r)) |
848 | 237 | 236 | ||
849 | 238 | if "manifest_yaml" not in rev: | 237 | if "manifest_yaml" not in rev: |
850 | 239 | _add_error( | 238 | _add_error( |
851 | @@ -359,11 +358,11 @@ def get_staged_and_build_packages_from_manifest(m): | |||
852 | 359 | If not, obtain it from stage-packages for various parts instead | 358 | If not, obtain it from stage-packages for various parts instead |
853 | 360 | """ | 359 | """ |
854 | 361 | if "parts" not in m: | 360 | if "parts" not in m: |
856 | 362 | debug("Could not find 'parts' in manifest") | 361 | logger.debug("Could not find 'parts' in manifest") |
857 | 363 | return None | 362 | return None |
858 | 364 | 363 | ||
859 | 365 | if not m["parts"]: | 364 | if not m["parts"]: |
861 | 366 | debug("'parts' exists in manifest but it is empty") | 365 | logger.debug("'parts' exists in manifest but it is empty") |
862 | 367 | return None | 366 | return None |
863 | 368 | 367 | ||
864 | 369 | d = {} | 368 | d = {} |
865 | @@ -407,18 +406,18 @@ def get_staged_and_build_packages_from_manifest(m): | |||
866 | 407 | if len(d) == 0: | 406 | if len(d) == 0: |
867 | 408 | return None | 407 | return None |
868 | 409 | 408 | ||
870 | 410 | debug("\n" + pprint.pformat(d)) | 409 | logger.debug("\n" + pprint.pformat(d)) |
871 | 411 | return d | 410 | return d |
872 | 412 | 411 | ||
873 | 413 | 412 | ||
874 | 414 | def get_staged_packages_from_rock_manifest(m): | 413 | def get_staged_packages_from_rock_manifest(m): |
875 | 415 | """Obtain list of packages in stage-packages if section is present.""" | 414 | """Obtain list of packages in stage-packages if section is present.""" |
876 | 416 | if "stage-packages" not in m: | 415 | if "stage-packages" not in m: |
878 | 417 | debug("Could not find 'stage-packages' in manifest") | 416 | logger.debug("Could not find 'stage-packages' in manifest") |
879 | 418 | return None | 417 | return None |
880 | 419 | 418 | ||
881 | 420 | if not m["stage-packages"]: | 419 | if not m["stage-packages"]: |
883 | 421 | debug("'stage-packages' exists in manifest but it is empty") | 420 | logger.debug("'stage-packages' exists in manifest but it is empty") |
884 | 422 | return None | 421 | return None |
885 | 423 | 422 | ||
886 | 424 | d = {} | 423 | d = {} |
887 | @@ -426,7 +425,7 @@ def get_staged_packages_from_rock_manifest(m): | |||
888 | 426 | if len(d) == 0: | 425 | if len(d) == 0: |
889 | 427 | return None | 426 | return None |
890 | 428 | 427 | ||
892 | 429 | debug("\n" + pprint.pformat(d)) | 428 | logger.debug("\n" + pprint.pformat(d)) |
893 | 430 | return d | 429 | return d |
894 | 431 | 430 | ||
895 | 432 | 431 | ||
896 | @@ -440,11 +439,11 @@ def get_packages_from_manifest_section(d, manifest_section, package_type): | |||
897 | 440 | """ | 439 | """ |
898 | 441 | for entry in manifest_section: | 440 | for entry in manifest_section: |
899 | 442 | if "=" not in entry: | 441 | if "=" not in entry: |
901 | 443 | warn("'%s' not properly formatted. Skipping" % entry) | 442 | logger.warning("'%s' not properly formatted. Skipping" % entry) |
902 | 444 | continue | 443 | continue |
903 | 445 | pkg, ver = entry.split("=") | 444 | pkg, ver = entry.split("=") |
904 | 446 | if pkg in update_binaries_ignore: | 445 | if pkg in update_binaries_ignore: |
906 | 447 | debug("Skipping ignored binary: '%s'" % pkg) | 446 | logger.debug("Skipping ignored binary: '%s'" % pkg) |
907 | 448 | continue | 447 | continue |
908 | 449 | if package_type not in d: | 448 | if package_type not in d: |
909 | 450 | d[package_type] = {} | 449 | d[package_type] = {} |
910 | @@ -467,7 +466,7 @@ def get_packages_from_rock_manifest_section(d, manifest_section, package_type): | |||
911 | 467 | # rock manifest v1 stage-packages section is a list of | 466 | # rock manifest v1 stage-packages section is a list of |
912 | 468 | # "<binpkg1>=<version>,<srcpkg1>=<src version>" | 467 | # "<binpkg1>=<version>,<srcpkg1>=<src version>" |
913 | 469 | if "=" not in entry or "," not in entry: | 468 | if "=" not in entry or "," not in entry: |
915 | 470 | warn("'%s' not properly formatted. Skipping" % entry) | 469 | logger.warning("'%s' not properly formatted. Skipping" % entry) |
916 | 471 | continue | 470 | continue |
917 | 472 | pkg_info = entry.split(",") | 471 | pkg_info = entry.split(",") |
918 | 473 | if len(pkg_info) == 2: | 472 | if len(pkg_info) == 2: |
919 | @@ -485,7 +484,7 @@ def get_packages_from_rock_manifest_section(d, manifest_section, package_type): | |||
920 | 485 | binary_pkg_name = arch_qualifier_parts[0] | 484 | binary_pkg_name = arch_qualifier_parts[0] |
921 | 486 | ver = binary_pkg[1] | 485 | ver = binary_pkg[1] |
922 | 487 | if binary_pkg_name in update_binaries_ignore: | 486 | if binary_pkg_name in update_binaries_ignore: |
924 | 488 | debug("Skipping ignored binary: '%s'" % binary_pkg_name) | 487 | logger.debug("Skipping ignored binary: '%s'" % binary_pkg_name) |
925 | 489 | continue | 488 | continue |
926 | 490 | if package_type not in d: | 489 | if package_type not in d: |
927 | 491 | d[package_type] = {} | 490 | d[package_type] = {} |
928 | @@ -494,10 +493,10 @@ def get_packages_from_rock_manifest_section(d, manifest_section, package_type): | |||
929 | 494 | if ver not in d[package_type][binary_pkg_name]: | 493 | if ver not in d[package_type][binary_pkg_name]: |
930 | 495 | d[package_type][binary_pkg_name].append(ver) | 494 | d[package_type][binary_pkg_name].append(ver) |
931 | 496 | else: | 495 | else: |
933 | 497 | warn("'%s' not properly formatted. Skipping" % pkg_info) | 496 | logger.warning("'%s' not properly formatted. Skipping" % pkg_info) |
934 | 498 | continue | 497 | continue |
935 | 499 | else: | 498 | else: |
937 | 500 | warn("'%s' not properly formatted. Skipping" % pkg_info) | 499 | logger.warning("'%s' not properly formatted. Skipping" % pkg_info) |
938 | 501 | continue | 500 | continue |
939 | 502 | 501 | ||
940 | 503 | 502 | ||
941 | @@ -544,7 +543,7 @@ def get_secnots_for_manifest( | |||
942 | 544 | ignore_pockets=list(), | 543 | ignore_pockets=list(), |
943 | 545 | ): | 544 | ): |
944 | 546 | """Find new security notifications for packages in the manifest""" | 545 | """Find new security notifications for packages in the manifest""" |
946 | 547 | debug(manifest_type + "/manifest.yaml:\n" + pprint.pformat(manifest)) | 546 | logger.debug(manifest_type + "/manifest.yaml:\n" + pprint.pformat(manifest)) |
947 | 548 | stage_and_build_pkgs = None | 547 | stage_and_build_pkgs = None |
948 | 549 | rel = get_ubuntu_release_from_manifest( | 548 | rel = get_ubuntu_release_from_manifest( |
949 | 550 | manifest, manifest_type | 549 | manifest, manifest_type |
950 | @@ -561,7 +560,7 @@ def get_secnots_for_manifest( | |||
951 | 561 | 560 | ||
952 | 562 | pending_secnots = {} | 561 | pending_secnots = {} |
953 | 563 | if stage_and_build_pkgs is None: | 562 | if stage_and_build_pkgs is None: |
955 | 564 | debug("no stage-packages found") | 563 | logger.debug("no stage-packages found") |
956 | 565 | return pending_secnots | 564 | return pending_secnots |
957 | 566 | # Since stage_and_build_pkgs can have stage-packages and build-packages | 565 | # Since stage_and_build_pkgs can have stage-packages and build-packages |
958 | 567 | # keys, adding secnots into each group | 566 | # keys, adding secnots into each group |
959 | @@ -594,7 +593,7 @@ def get_secnots_for_manifest( | |||
960 | 594 | if key in manifest["parts"][part]: | 593 | if key in manifest["parts"][part]: |
961 | 595 | for entry in manifest["parts"][part][key]: | 594 | for entry in manifest["parts"][part][key]: |
962 | 596 | if "=" not in entry: | 595 | if "=" not in entry: |
964 | 597 | warn( | 596 | logger.warning( |
965 | 598 | "'%s' not properly " | 597 | "'%s' not properly " |
966 | 599 | "formatted. Skipping" % entry | 598 | "formatted. Skipping" % entry |
967 | 600 | ) | 599 | ) |
968 | @@ -616,7 +615,7 @@ def get_secnots_for_manifest( | |||
969 | 616 | secnotversion = secnot_db[rel][pkg][secnot]["version"] | 615 | secnotversion = secnot_db[rel][pkg][secnot]["version"] |
970 | 617 | 616 | ||
971 | 618 | if debversion.compare(pkgversion, secnotversion) < 0: | 617 | if debversion.compare(pkgversion, secnotversion) < 0: |
973 | 619 | debug( | 618 | logger.debug( |
974 | 620 | "adding %s: %s (pkg:%s < secnot:%s)" | 619 | "adding %s: %s (pkg:%s < secnot:%s)" |
975 | 621 | % ( | 620 | % ( |
976 | 622 | pkg, | 621 | pkg, |
977 | @@ -641,7 +640,7 @@ def get_secnots_for_manifest( | |||
978 | 641 | else: | 640 | else: |
979 | 642 | pending_secnots[pkg_type][pkg].append(secnot) | 641 | pending_secnots[pkg_type][pkg].append(secnot) |
980 | 643 | else: | 642 | else: |
982 | 644 | debug( | 643 | logger.debug( |
983 | 645 | "skipping %s: %s (pkg:%s >= secnot:%s)" | 644 | "skipping %s: %s (pkg:%s >= secnot:%s)" |
984 | 646 | % ( | 645 | % ( |
985 | 647 | pkg, | 646 | pkg, |
986 | @@ -675,7 +674,7 @@ def get_ubuntu_release_from_manifest_snap(m): | |||
987 | 675 | if "installed-snaps" in m["parts"][part]: | 674 | if "installed-snaps" in m["parts"][part]: |
988 | 676 | for entry in m["parts"][part]["installed-snaps"]: | 675 | for entry in m["parts"][part]["installed-snaps"]: |
989 | 677 | if "=" not in entry: | 676 | if "=" not in entry: |
991 | 678 | warn("'%s' not properly formatted. Skipping" % entry) | 677 | logger.warning("'%s' not properly formatted. Skipping" % entry) |
992 | 679 | continue | 678 | continue |
993 | 680 | pkg = entry.split("=")[0] # we don't care about the version | 679 | pkg = entry.split("=")[0] # we don't care about the version |
994 | 681 | 680 | ||
995 | @@ -689,7 +688,7 @@ def get_ubuntu_release_from_manifest_snap(m): | |||
996 | 689 | ubuntu_release = get_os_codename( | 688 | ubuntu_release = get_os_codename( |
997 | 690 | m["snapcraft-os-release-id"], m["snapcraft-os-release-version-id"] | 689 | m["snapcraft-os-release-id"], m["snapcraft-os-release-version-id"] |
998 | 691 | ) | 690 | ) |
1000 | 692 | debug("Ubuntu release=%s" % ubuntu_release) | 691 | logger.debug("Ubuntu release=%s" % ubuntu_release) |
1001 | 693 | return ubuntu_release | 692 | return ubuntu_release |
1002 | 694 | except ValueError: | 693 | except ValueError: |
1003 | 695 | pass | 694 | pass |
1004 | @@ -720,7 +719,7 @@ def get_ubuntu_release_from_manifest_snap(m): | |||
1005 | 720 | ubuntu_release = get_os_codename( | 719 | ubuntu_release = get_os_codename( |
1006 | 721 | "ubuntu", m["snapcraft-os-release-version-id"] | 720 | "ubuntu", m["snapcraft-os-release-version-id"] |
1007 | 722 | ) | 721 | ) |
1009 | 723 | debug( | 722 | logger.debug( |
1010 | 724 | "Could not determine Ubuntu release (non-core base)." | 723 | "Could not determine Ubuntu release (non-core base)." |
1011 | 725 | "Guessing based on snapcraft-os-release-version-id: %s" | 724 | "Guessing based on snapcraft-os-release-version-id: %s" |
1012 | 726 | % (m["snapcraft-os-release-version-id"]) | 725 | % (m["snapcraft-os-release-version-id"]) |
1013 | @@ -731,7 +730,7 @@ def get_ubuntu_release_from_manifest_snap(m): | |||
1014 | 731 | if ubuntu_release is None: | 730 | if ubuntu_release is None: |
1015 | 732 | # no installed snaps, use default | 731 | # no installed snaps, use default |
1016 | 733 | if len(installed_snaps) == 0: | 732 | if len(installed_snaps) == 0: |
1018 | 734 | debug( | 733 | logger.debug( |
1019 | 735 | "Could not determine Ubuntu release (non-core base with " | 734 | "Could not determine Ubuntu release (non-core base with " |
1020 | 736 | "no " | 735 | "no " |
1021 | 737 | "installed snaps). Defaulting to '%s'" % default | 736 | "installed snaps). Defaulting to '%s'" % default |
1022 | @@ -757,7 +756,7 @@ def get_ubuntu_release_from_manifest_rock(m): | |||
1023 | 757 | ubuntu_release = get_os_codename( | 756 | ubuntu_release = get_os_codename( |
1024 | 758 | m["os-release-id"], str(m["os-release-version-id"]) | 757 | m["os-release-id"], str(m["os-release-version-id"]) |
1025 | 759 | ) | 758 | ) |
1027 | 760 | debug("Ubuntu release=%s" % ubuntu_release) | 759 | logger.debug("Ubuntu release=%s" % ubuntu_release) |
1028 | 761 | except ValueError: | 760 | except ValueError: |
1029 | 762 | pass | 761 | pass |
1030 | 763 | else: | 762 | else: |
1031 | @@ -775,7 +774,7 @@ def get_ubuntu_release_from_manifest(m, m_type="snap"): | |||
1032 | 775 | ubuntu_release = get_ubuntu_release_from_manifest_rock(m) | 774 | ubuntu_release = get_ubuntu_release_from_manifest_rock(m) |
1033 | 776 | else: | 775 | else: |
1034 | 777 | raise TypeError("Unsupported manifest type %s" % m_type) | 776 | raise TypeError("Unsupported manifest type %s" % m_type) |
1036 | 778 | debug("Ubuntu release=%s" % ubuntu_release) | 777 | logger.debug("Ubuntu release=%s" % ubuntu_release) |
1037 | 779 | 778 | ||
1038 | 780 | return ubuntu_release | 779 | return ubuntu_release |
1039 | 781 | 780 | ||
1040 | @@ -789,5 +788,5 @@ def get_snapcraft_version_from_manifest(m): | |||
1041 | 789 | try: | 788 | try: |
1042 | 790 | return debversion.DebVersion(m["snapcraft-version"]) | 789 | return debversion.DebVersion(m["snapcraft-version"]) |
1043 | 791 | except ValueError as e: | 790 | except ValueError as e: |
1045 | 792 | warn("Invalid Snapcraft version: %s" % e) | 791 | logger.warning("Invalid Snapcraft version: %s" % e) |
1046 | 793 | return None | 792 | return None |
1047 | diff --git a/tests/test-dump-tool.sh.expected b/tests/test-dump-tool.sh.expected | |||
1048 | index 702666e..1cc4104 100644 | |||
1049 | --- a/tests/test-dump-tool.sh.expected | |||
1050 | +++ b/tests/test-dump-tool.sh.expected | |||
1051 | @@ -14,14 +14,14 @@ Done: 10 revisions read | |||
1052 | 14 | 14 | ||
1053 | 15 | = Test --file --warn-if-empty = | 15 | = Test --file --warn-if-empty = |
1054 | 16 | Running: dump-tool --warn-if-empty --file ./tests/test-store-dump.1 | 16 | Running: dump-tool --warn-if-empty --file ./tests/test-store-dump.1 |
1063 | 17 | WARN: Skipping 4ZWSHtNHDBdBsoGCTVgKkk8F9MtvjZS1|1: empty | 17 | WARNING: Skipping 4ZWSHtNHDBdBsoGCTVgKkk8F9MtvjZS1|1: empty |
1064 | 18 | WARN: Skipping 4ZWSHtNHDBdBsoGCTVgKkk8F9MtvjZS1|2: empty | 18 | WARNING: Skipping 4ZWSHtNHDBdBsoGCTVgKkk8F9MtvjZS1|2: empty |
1065 | 19 | WARN: Skipping 4ZWSHtNHDBdBsoGCTVgKkk8F9MtvjZS1|3: empty | 19 | WARNING: Skipping 4ZWSHtNHDBdBsoGCTVgKkk8F9MtvjZS1|3: empty |
1066 | 20 | WARN: Skipping 99T7MUlRhtI3U0QFgl5mXXESAiSwt776|1: empty | 20 | WARNING: Skipping 99T7MUlRhtI3U0QFgl5mXXESAiSwt776|1: empty |
1067 | 21 | WARN: Skipping UqFziVZDHLSyO3TqSWgNBoAdHbLI4dAH|1: empty | 21 | WARNING: Skipping UqFziVZDHLSyO3TqSWgNBoAdHbLI4dAH|1: empty |
1068 | 22 | WARN: Skipping UqFziVZDHLSyO3TqSWgNBoAdHbLI4dAH|2: empty | 22 | WARNING: Skipping UqFziVZDHLSyO3TqSWgNBoAdHbLI4dAH|2: empty |
1069 | 23 | WARN: Skipping UqFziVZDHLSyO3TqSWgNBoAdHbLI4dAH|3: empty | 23 | WARNING: Skipping UqFziVZDHLSyO3TqSWgNBoAdHbLI4dAH|3: empty |
1070 | 24 | WARN: Skipping UqFziVZDHLSyO3TqSWgNBoAdHbLI4dAH|4: empty | 24 | WARNING: Skipping UqFziVZDHLSyO3TqSWgNBoAdHbLI4dAH|4: empty |
1071 | 25 | 25 | ||
1072 | 26 | = Test --db-file = | 26 | = Test --db-file = |
1073 | 27 | Running: dump-tool --db-file ./tests/test-store-dump.1.json | 27 | Running: dump-tool --db-file ./tests/test-store-dump.1.json |
1074 | diff --git a/tests/test-snap-verify-declaration.sh.expected b/tests/test-snap-verify-declaration.sh.expected | |||
1075 | index 0149682..88f34ee 100644 | |||
1076 | --- a/tests/test-snap-verify-declaration.sh.expected | |||
1077 | +++ b/tests/test-snap-verify-declaration.sh.expected | |||
1078 | @@ -4,7 +4,7 @@ usage: snap-verify-declaration [-h] [--json] [--plugs PLUGS] [--slots SLOTS] | |||
1079 | 4 | 4 | ||
1080 | 5 | Check a snap declaration for errors | 5 | Check a snap declaration for errors |
1081 | 6 | 6 | ||
1083 | 7 | optional arguments: | 7 | options: |
1084 | 8 | -h, --help show this help message and exit | 8 | -h, --help show this help message and exit |
1085 | 9 | --json print json output | 9 | --json print json output |
1086 | 10 | --plugs PLUGS file specifying snap declaration for plugs | 10 | --plugs PLUGS file specifying snap declaration for plugs |
1087 | @@ -138,18 +138,7 @@ Running: snap-verify-declaration --plugs=./plugs | |||
1088 | 138 | ERROR: Failed to read plugs: Expecting property name enclosed in double quotes: line 2 column 1 (char 2) | 138 | ERROR: Failed to read plugs: Expecting property name enclosed in double quotes: line 2 column 1 (char 2) |
1089 | 139 | 139 | ||
1090 | 140 | Running: snap-verify-declaration --json --plugs=./plugs | 140 | Running: snap-verify-declaration --json --plugs=./plugs |
1103 | 141 | { | 141 | ERROR: Failed to read plugs: Expecting property name enclosed in double quotes: line 2 column 1 (char 2) |
1092 | 142 | "runtime-errors": { | ||
1093 | 143 | "error": { | ||
1094 | 144 | "msg": { | ||
1095 | 145 | "manual_review": true, | ||
1096 | 146 | "text": "Failed to read plugs: Expecting property name enclosed in double quotes: line 2 column 1 (char 2)" | ||
1097 | 147 | } | ||
1098 | 148 | }, | ||
1099 | 149 | "info": {}, | ||
1100 | 150 | "warn": {} | ||
1101 | 151 | } | ||
1102 | 152 | } | ||
1104 | 153 | 142 | ||
1105 | 154 | Running: snap-verify-declaration --plugs=./plugs | 143 | Running: snap-verify-declaration --plugs=./plugs |
1106 | 155 | {'error': {'snap-declaration-verify_v2:valid_interface:nonexistent': {'manual_review': False, | 144 | {'error': {'snap-declaration-verify_v2:valid_interface:nonexistent': {'manual_review': False, |
1107 | @@ -208,16 +197,5 @@ Running: snap-verify-declaration --plugs=./plugs | |||
1108 | 208 | ERROR: Failed to read plugs: bare <class 'bool'> value True for 'mir:allow-auto-connection' is not allowed - this should be a string "true" instead | 197 | ERROR: Failed to read plugs: bare <class 'bool'> value True for 'mir:allow-auto-connection' is not allowed - this should be a string "true" instead |
1109 | 209 | 198 | ||
1110 | 210 | Running: snap-verify-declaration --json --plugs=./plugs | 199 | Running: snap-verify-declaration --json --plugs=./plugs |
1123 | 211 | { | 200 | ERROR: Failed to read plugs: bare <class 'bool'> value True for 'mir:allow-auto-connection' is not allowed - this should be a string "true" instead |
1112 | 212 | "runtime-errors": { | ||
1113 | 213 | "error": { | ||
1114 | 214 | "msg": { | ||
1115 | 215 | "manual_review": true, | ||
1116 | 216 | "text": "Failed to read plugs: bare <class 'bool'> value True for 'mir:allow-auto-connection' is not allowed - this should be a string \"true\" instead" | ||
1117 | 217 | } | ||
1118 | 218 | }, | ||
1119 | 219 | "info": {}, | ||
1120 | 220 | "warn": {} | ||
1121 | 221 | } | ||
1122 | 222 | } | ||
1124 | 223 | 201 | ||
1125 | diff --git a/tests/test-updates-available.sh.expected b/tests/test-updates-available.sh.expected | |||
1126 | index f6957fe..47edb1c 100644 | |||
1127 | --- a/tests/test-updates-available.sh.expected | |||
1128 | +++ b/tests/test-updates-available.sh.expected | |||
1129 | @@ -1419,7 +1419,7 @@ Running: snap-updates-available --with-cves --usn-db='./tests/test-usn-unittest- | |||
1130 | 1419 | } | 1419 | } |
1131 | 1420 | 1420 | ||
1132 | 1421 | Running: ./bin/snap-updates-available --usn-db='./tests/test-usn-unittest-1.db' --store-db='./tests/test-store-unittest-bad-1.db' | 1421 | Running: ./bin/snap-updates-available --usn-db='./tests/test-usn-unittest-1.db' --store-db='./tests/test-store-unittest-bad-1.db' |
1134 | 1422 | WARN: 1ad: required field 'revisions' not found | 1422 | WARNING: 1ad: required field 'revisions' not found |
1135 | 1423 | ERROR: Errors encountered when scanning store entries | 1423 | ERROR: Errors encountered when scanning store entries |
1136 | 1424 | From: Snap Store <noreply+security-snap-review@canonical.com> | 1424 | From: Snap Store <noreply+security-snap-review@canonical.com> |
1137 | 1425 | To: olivier.tilloy@canonical.com | 1425 | To: olivier.tilloy@canonical.com |
1138 | @@ -1655,10 +1655,10 @@ References: | |||
1139 | 1655 | 1655 | ||
1140 | 1656 | 1656 | ||
1141 | 1657 | Running: ./bin/snap-updates-available --usn-db='./tests/test-usn-unittest-build-pkgs.db' --store-db='./tests/test-store-unittest-invalid-snapcraft-version.db' | 1657 | Running: ./bin/snap-updates-available --usn-db='./tests/test-usn-unittest-build-pkgs.db' --store-db='./tests/test-store-unittest-invalid-snapcraft-version.db' |
1143 | 1658 | WARN: Invalid Snapcraft version: '4.4.3' not a valid Debian version | 1658 | WARNING: Invalid Snapcraft version: '4.4.3' not a valid Debian version |
1144 | 1659 | 1659 | ||
1145 | 1660 | Running: ./bin/snap-updates-available --usn-db='./tests/test-usn-unittest-build-pkgs.db' --store-db='./tests/test-store-kernel-invalid-snapcraft-version.db' | 1660 | Running: ./bin/snap-updates-available --usn-db='./tests/test-usn-unittest-build-pkgs.db' --store-db='./tests/test-store-kernel-invalid-snapcraft-version.db' |
1147 | 1661 | WARN: Invalid Snapcraft version: '2.3' not a valid Debian version | 1661 | WARNING: Invalid Snapcraft version: '2.3' not a valid Debian version |
1148 | 1662 | 1662 | ||
1149 | 1663 | Running: snap-updates-available --usn-db='./tests/test-usn-unittest-lp1841848.db' --snap='./tests/test-check-notices_0.1_amd64.snap' | 1663 | Running: snap-updates-available --usn-db='./tests/test-usn-unittest-lp1841848.db' --snap='./tests/test-check-notices_0.1_amd64.snap' |
1150 | 1664 | 1664 |
This MR slighly changes the output format under certain circumstances. It removes the ability to return json formatted errors for errors generated by the cli script caused during the cli arguments validation/ processing. This behaviour looks desirable to me anyway tbh
See inline comments!