suite.Run.schedule_jobs: Unmask error in dry-run #1958
Conversation
For whatever reason we were suppressing an error related to unavailable packages. Signed-off-by: Zack Cerza <[email protected]>
And clean up the code a bit Signed-off-by: Zack Cerza <[email protected]>
| not self.args.dry_run and | ||
| not config.suite_allow_missing_packages | ||
| ): | ||
| if not config.suite_allow_missing_packages: | ||
| util.schedule_fail( |
There was a problem hiding this comment.
Just thinking about the user impact...it would be better to identify all the jobs clearly that are missing packages when --dry-running. Would it be better to report something identifying the job and continue, rather than calling schedule_fail here? In a non-dry-run, failure makes sense (although arguably it ought to fail when there are any jobs missing packages), but in dry-run, reporting as much as possible seems better. wdyt?
There was a problem hiding this comment.
It felt important to converge the behavior of the two modes, since users seemed pretty surprised that they differed. I do think we could stand to include a little more information though, like what specifically was missing
| log.error('can''t find %d parents of %s in %s: %s', | ||
| int(count), sha1, project, resp.json()['error']) | ||
| resp_json = resp.json() | ||
| err_msg = resp_json.get("error") or resp_json.get("err") |
There was a problem hiding this comment.
@zmc I wonder why have you left get("error")?
Also fix a KeyError.