-
-
Notifications
You must be signed in to change notification settings - Fork 55
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.
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
Improve the Release status dashboard #547
base: main
Are you sure you want to change the base?
Conversation
Quick branch info (status, RM name, release schedule link) for all branches at the top, with detailed per-branch sections after that. Details can generally be expanded/collapsed. Show tier-3+ failures: these aren't considered release-blocking but we tend to fix these ASAP. These mark a branch/builder "yellow". Show low-priority problems: disconnected builders, build warnings, unstable builder failures. These keep a branch marked "green". Show changes and test results for failing builds (when available). Show build history overview for builders; this is useful context. Conceptually, the branch sections list "problems", not "builders", in preparation for adding GitHub blocker issues here. Problems are sorted by severity & grouped by type; the highest-severity problem determines the overall branch status. Based on Pablo's existing dashboard and his BuildbotHammer tool. Co-Authored-By: Pablo Galindo Salgado <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments for a first pass
master/custom/release_dashboard.py
Outdated
try: | ||
return func(*args, **kwargs) | ||
except AttributeError as e: | ||
raise Exception(f'your error: {e!r}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe raise something more concrete (maybe TemplateAttributeError
or something else).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
(This is a debug helper -- but one that I find myself using constantly so I'd rather leave it in.)
master/custom/release_dashboard.py
Outdated
except IndexError: | ||
return NoProblem() | ||
except AttributeError: | ||
raise SystemError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why SystemError
? Shouldn't better be something concrete likeBuilderError
or something akin to that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I left some debug code in. Thanks for the catch!
master/custom/release_dashboard.py
Outdated
def junit_results(self): | ||
filepath = ( | ||
self._root._app.test_result_dir | ||
/ self.builder.branch.tag | ||
/ self.builder["name"] | ||
/ f'build_{self["number"]}.xml' | ||
) | ||
try: | ||
file = filepath.open() | ||
except OSError: | ||
return None | ||
with file: | ||
etree = ElementTree.parse(file) | ||
result = JunitResult(self, {}) | ||
for element in etree.iterfind('.//error/..'): | ||
result.add(element) | ||
return result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This assumes there is a logger somewhere, but you can drop that)
def junit_results(self): | |
filepath = ( | |
self._root._app.test_result_dir | |
/ self.builder.branch.tag | |
/ self.builder["name"] | |
/ f'build_{self["number"]}.xml' | |
) | |
try: | |
file = filepath.open() | |
except OSError: | |
return None | |
with file: | |
etree = ElementTree.parse(file) | |
result = JunitResult(self, {}) | |
for element in etree.iterfind('.//error/..'): | |
result.add(element) | |
return result | |
@cached_property | |
def junit_results(self): | |
if not self._root._app.test_result_dir: | |
return None | |
try: | |
filepath = ( | |
self._root._app.test_result_dir | |
/ self.builder.branch.tag | |
/ self.builder["name"] | |
/ f'build_{self["number"]}.xml' | |
).resolve() | |
# Ensure path doesn't escape test_result_dir | |
if not str(filepath).startswith(str(self._root._app.test_result_dir)): | |
logger.warning(f"Attempted path traversal: {filepath}") | |
return None | |
if not filepath.is_file(): | |
return None | |
with filepath.open() as file: | |
etree = ElementTree.parse(file) | |
except OSError as e: | |
logger.error(f"Failed to read JUnit results: {e}") | |
return None | |
except ElementTree.ParseError as e: | |
logger.error(f"Failed to parse JUnit XML: {e}") | |
raise XMLParseError(f"Malformed JUnit XML: {e}") from e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I'll punt on logging in this PR.
if error not in result.errors: | ||
# De-duplicate, since failing tests are re-run and often fail | ||
# the same way | ||
result.errors.extend(errors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hummmm aren'y you adding technically all the errors again and again once per error to the list? What am I missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this is confusing. I'll add comments.
-
This is called per
<There's usually only one error in
errorsand
error_types. (With
subTest`, there should IMO be multiple errors per test, but we don't do that yet. I opened libregrtest: junit-xml output doesn't support subTest cpython#126237 for it, so it's not sitting only in my TODO list.) -
The types are added to each level of the tree, so that if you don't expand the details you see list of all (unique) exception types under it.
Wow Petr, this is absolutely fantastic work! 🎉 I'm really excited to see the release dashboard coming together like this! This is going to make everyone's lives so much easier. I made a first pass but given how big this PR is I probably missed stuff so I may do one more later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your kind words!
Do you think it's OK to replace the existing dashbuard, or should we leave them side-by-side for a while?
master/custom/release_dashboard.py
Outdated
except IndexError: | ||
return NoProblem() | ||
except AttributeError: | ||
raise SystemError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I left some debug code in. Thanks for the catch!
if error not in result.errors: | ||
# De-duplicate, since failing tests are re-run and often fail | ||
# the same way | ||
result.errors.extend(errors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this is confusing. I'll add comments.
-
This is called per
<There's usually only one error in
errorsand
error_types. (With
subTest`, there should IMO be multiple errors per test, but we don't do that yet. I opened libregrtest: junit-xml output doesn't support subTest cpython#126237 for it, so it's not sitting only in my TODO list.) -
The types are added to each level of the tree, so that if you don't expand the details you see list of all (unique) exception types under it.
master/custom/release_dashboard.py
Outdated
def junit_results(self): | ||
filepath = ( | ||
self._root._app.test_result_dir | ||
/ self.builder.branch.tag | ||
/ self.builder["name"] | ||
/ f'build_{self["number"]}.xml' | ||
) | ||
try: | ||
file = filepath.open() | ||
except OSError: | ||
return None | ||
with file: | ||
etree = ElementTree.parse(file) | ||
result = JunitResult(self, {}) | ||
for element in etree.iterfind('.//error/..'): | ||
result.add(element) | ||
return result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I'll punt on logging in this PR.
master/custom/release_dashboard.py
Outdated
try: | ||
return func(*args, **kwargs) | ||
except AttributeError as e: | ||
raise Exception(f'your error: {e!r}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
(This is a debug helper -- but one that I find myself using constantly so I'd rather leave it in.)
I've spent some time making the Release Dashboard closer to my daily status reports (and include the info needed for them). Here's the result.
Outdated static demo: https://encukou.github.io/buildbot-status-dashboard/
WIP repo, with caching and hacks needed to iterate on this locally: https://github.com/encukou/buildbot-status-dashboard
Technically it should be drop-in upgrade for the existing dashboard, and that's what this PR does. But I now think it would be better to have both, at least for a while, in case this falls over. But, code is here.
Does this look reasonable?
Quick branch info (status, RM name, release schedule link) for all branches at the top, with detailed per-branch sections after that.
Details can generally be expanded/collapsed.
Show tier-3+ failures: these aren't considered release-blocking but we tend to fix these ASAP. These mark a "green" branch/builder as "yellow".
Show low-priority problems: disconnected builders, build warnings, unstable builder failures. These don't affect the overall status.
Show changes and test results for failing builds (when available).
Show build history overview for builders; this is useful context.
Conceptually, the branch sections list "problems", not builders, in preparation for adding blocker issues from GitHub. Problems are sorted by severity & grouped by type; the highest-severity problem determines the overall branch status.
Pull Request builders are, for now, shown in a
no-branch
section (hiding at the end with no link to it).Based on Pablo's existing dashboard and his BuildbotHammer tool.