-
-
Notifications
You must be signed in to change notification settings - Fork 25
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
Parse evaluation warnings and display in separate section. #180
base: main
Are you sure you want to change the base?
Conversation
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.
nice, this is on the right track
@@ -172,6 +172,11 @@ def run(self) -> Generator[Any, object, Any]: | |||
# if the command passes extract the list of stages | |||
result = cmd.results() | |||
if result == util.SUCCESS: | |||
log.info(self.observer.getStderr()) | |||
self.addHTMLLog( | |||
"Evaluation Warnings", f"<pre>{self.observer.getStderr()}</pre>" |
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.
Do you know if the string interpolation gets properly HTML-escaped?
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.
Uh, definitely not, good catch! JavaScript injection attacks disaprove :)
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.
I think the python stdlib has some helpers for that. Otherwise I am sure that buildbot has it somewhere.
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.
Indeed, but this was more of a prototype to showcase what can be done now anyway, but i don't think it's worth merging as it is, personally.
@@ -172,6 +172,11 @@ def run(self) -> Generator[Any, object, Any]: | |||
# if the command passes extract the list of stages | |||
result = cmd.results() | |||
if result == util.SUCCESS: | |||
log.info(self.observer.getStderr()) |
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.
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.
Whitespace is already fixed, left over from a test and me not knowing how pre
works. But i dont think we can autoexpand, ill look. But to not put it there at all, would need UI changes
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.
Yeah i looked, we cant autoexpand without hacks. There is API to edit the UI, only to replace it :/
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.
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.
I will check, but I dont have high hopes
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.
Builbots UI is very configurable, if you want to add/replace views, not if you want to modify them... i'll join their IRC/something and ask, in case I missed something
Anything more than this is blocked by #173 and further discussion is needed, as buildbot does not allow partial UI replacements as far as I can tell, therefore we cannot add another element into the build UI. |
Unless the eval error gets more visibility, it makes sense to table this for now. |
@mergify rebase |
Signed-off-by: magic_rb <[email protected]>
✅ Branch has been successfully rebased |
98c63d8
to
1788b41
Compare
This is superseded by #255 as now evaluation failures appear as separate builds. |
But is this the same as warnings though? |
|
Ah yeah if the build doesn't fail it doesn't show. That's my mistake. |
A live version can be seen here and in case my buildbot dies or gets reset, a screenshot:
Possibly fixes #172, for anything more fancy we'll need #173, what do you think @zimbatm?
And due to the exploration here, I've learned that we can add arbitrary HTML to the page, as long as it is contained in one of those tabs. Well, injecting javascript and modifying the page is an option, but a really bad one :)