-
Notifications
You must be signed in to change notification settings - Fork 6
Add support for standalone reporting command #108
Conversation
Pull Request Test Coverage Report for Build 390
💛 - Coveralls |
dec3fdd
to
6647787
Compare
This is more or less ready to review. |
639cbe7
to
a99a841
Compare
Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
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 this seems fine in general as it is opt-in and won't cause issues with current pipeline.
I just had some concerns about input. Everything else seems straightforward.
sktm/reporter.py
Outdated
for header in headers: | ||
key, value = header.split(':', 1) | ||
self.report[key] = value | ||
|
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.
Who sanitizes the input? If I type --header foo --header bar:blah, I think things will go bad? Or is the input files verified?
Not that many non-scripts will touch those cli options, but in case things get fat fingered..
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 logic is taken from skt (but can be changed if needed). Missing space as in your second example is not a problem, only missing :
is (as per your first example). The format is described in the help string if someone wants to use the command manually. IMHO, things should "go bad" if you pass invalid option. The error thrown should show the line which tries to split the header so it should be obvious what is the problem but if you would feel more comfortable with it, I can add a custom error message saying that the header passed doesn't conform to the required format.
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.
Ok. If this is more-or-less a copy-n-paste of skt, then it is probably fine. Eventually, it would be nice to have some checking going on to avoid silly bugs. I wonder if a simple 'if not value: raise Exception()' works here.
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.
Not really, since the assignment itself will throw the exception. But I added a custom message to it, please check if that's what you meant
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. thanks.
Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
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.
Seems fine to me.
sktm/reporter.py
Outdated
for header in headers: | ||
key, value = header.split(':', 1) | ||
self.report[key] = value | ||
|
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. thanks.
Test with commands like
sktm -vv --mail-from me --mail-to someone report --assets dir-with-results
where
dir-with-results
is a directory containing stage results (which are not yet implemented in skt, so you'll need to create your own ones for testing purposes).Examples of content:
build.log build.report build.result merge.report merge.result
or
applied.patch merge.report merge.result s390/ x86/
wheres390/
containsbuild.report build.result config.gz kitty
andx86/
containsbuild.report build.result useless-file
From the examples,
useless-file
is not referenced in the report text so it's not added,config.gz
,applied.patch
andkitty
are referenced and added as attachments.I'll add a proper directory structure definition to meta/pull6 so it's less confusing but I wanted to publish the code here before the weekend.