Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

suite.Run.schedule_jobs: Unmask error in dry-run #1958

Merged
merged 2 commits into from
Jun 14, 2024
Merged

suite.Run.schedule_jobs: Unmask error in dry-run #1958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0da32ff
suite.Run.schedule_jobs: Unmask error in dry-run
zmc Jun 14, 2024
24b6915
suite.util.find_git_parents: Fix KeyError
zmc Jun 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions teuthology/suite/run.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,13 +512,10 @@ def schedule_jobs(self, jobs_missing_packages, jobs_to_schedule, name):
log_prefix = ''
if job in jobs_missing_packages:
log_prefix = "Missing Packages: "
if (
not self.args.dry_run and
not config.suite_allow_missing_packages
):
if not config.suite_allow_missing_packages:
util.schedule_fail(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"At least one job needs packages that don't exist for "
"hash {sha1}.".format(sha1=self.base_config.sha1),
"At least one job needs packages that don't exist "
f"for hash {self.base_config.sha1}.",
name,
dry_run=self.args.dry_run,
)
Expand Down
14 changes: 9 additions & 5 deletions teuthology/suite/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,15 +348,19 @@ def find_git_parents(project: str, sha1: str, count=1):
return []

def get_sha1s(project, committish, count):
url = '/'.join((base_url, '%s.git' % project,
'history/?committish=%s&count=%d' % (committish, count)))
url = '/'.join((
base_url,
f"{project}.git",
f"history/?committish={committish}&count={count}"
))
resp = requests.get(url)
resp.raise_for_status()
sha1s = resp.json()['sha1s']
if len(sha1s) != count:
log.debug('got response: %s', resp.json())
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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zmc I wonder why have you left get("error")?

log.debug(f"Got response: {resp_json}")
log.error(f"Can't find {count} parents of {sha1} in {project}: {err_msg}")
return sha1s

# index 0 will be the commit whose parents we want to find.
Expand Down
Loading