Automate incidents creation #586
Conversation
JenySadadia
force-pushed
the
auto-create-incidents
branch
2 times, most recently
from
e41e75b
to
b2c5392
Compare
|
@helen-fornazier Thanks for the comments. |
JenySadadia
force-pushed
the
auto-create-incidents
branch
2 times, most recently
from
7f7da72
to
0fe4538
Compare
JenySadadia
force-pushed
the
auto-create-incidents
branch
2 times, most recently
from
1a29241
to
a5f7531
Compare
|
Proposal for command line tool usage: with this, we can use this tool to debug, and also to validate a issue and test example in the editor UI before submitting it to kcidb @tales-aparecida @JenySadadia @spbnick what do you think? so we can drop all the options I had previously added there |
Following up this comment #586 (comment) I think we can leave it to a second PR, no problem for me |
JenySadadia
force-pushed
the
auto-create-incidents
branch
2 times, most recently
from
c01e741
to
0c1eee0
Compare
JenySadadia
force-pushed
the
auto-create-incidents
branch
from
0c1eee0
to
2c9a632
Compare
JenySadadia
force-pushed
the
auto-create-incidents
branch
from
2c9a632
to
0a1babc
Compare
JenySadadia
force-pushed
the
auto-create-incidents
branch
2 times, most recently
from
9552aec
to
094c755
Compare
There was a problem hiding this comment.
Sorry to leave a possibly overwhelming review, asking to change things a lot. Let's discuss this and see what's viable at this moment. However, this would all be for nothing if I don't fix that performance problem I was working on before Plumbers, and don't enable notifications. This code would simply not be called without that. So I'll go and work on that, instead of going further into detailed review 🙈
kcidb/tools/kcidb_match.py
Outdated
| If snippet_lines == 0: the full log | ||
| If snippet_lines > 0: the first snippet_lines log lines | ||
| If snippet_lines < 0: the last snippet_lines log lines | ||
| """ |
There was a problem hiding this comment.
Could you please document classes/methods/functions fully, including the arguments, following the conventions seen everywhere else in the code? Also, if you have a multi-line docstring, could you please start the text on the next line from the """? Thank you 🙏
kcidb/tools/kcidb_match.py
Outdated
| """ | ||
| try: | ||
| response = requests.get(url, timeout=60) | ||
| response.raise_for_status() |
There was a problem hiding this comment.
Do you think you could try to fetch the URL from the artifact cache first? Similarly to how it's done in the cache redirector:
Lines 576 to 577 in 37961dc
This way we could save a little of inbound traffic now, and once we have cache fully working we could save a lot.
kcidb/tools/kcidb_match.py
Outdated
| return None | ||
| try: | ||
| raw_bytes = gzip.decompress(response.content) | ||
| text = raw_bytes.decode('utf-8') |
There was a problem hiding this comment.
This should not be necessary, nor possible, as the log URL should point to a plain log file. This should be fixed in Maestro. E.g. by configuring the web server to specify Transfer-Encoding: gzip and serving the files compressed transparently. Python requests would then decompress that automatically. Otherwise this not only hurts our code complexity, but also the experience of users who download those files and then have to decompress them manually.
@nuclearcat, @pawiecz, how hard would that be to fix?
kcidb/tools/kcidb_match.py
Outdated
| """Pattern validator class""" | ||
| def __init__(self): | ||
| self.schema = copy.deepcopy(kcidb.io.SCHEMA.json) | ||
| self.remove_required_fields(self.schema) |
There was a problem hiding this comment.
This doesn't need to be a class at all. You don't need multiple of them and it doesn't have any parameters. Please remake it into a module (this or a separate one). Also, it doesn't only validate patterns.
| kcidb_io_object = {"tests": [test._data], | ||
| "builds": [test.build._data], | ||
| "checkouts": [test.build.checkout._data]} | ||
| return self.generate_incidents_from_db(kcidb_io_object) |
There was a problem hiding this comment.
You cannot make an I/O object from an OO object in general, even if it looks like you can. You just cannot count on that. An OO object is a processed I/O object, and there can be data loss. Instead deal with OO objects directly everywhere. Where you need to process I/O objects from stdin and command-line interface, load them into an sqlite database using the database client, get them as OO objects from there, and then process.
OO objects were specifically made to make the things you're doing here easier to do. Like walk related objects and so on. We can have a call this week and go over all the concerns and options regarding this.
There was a problem hiding this comment.
BTW, there's also a schema for the raw OO data in kcidb.orm.data.
| client = get_client() | ||
| if client: | ||
| incident_generator = kcidb_match.IncidentGenerator() | ||
| incidents = incident_generator.generate_incidents_from_test(test) |
There was a problem hiding this comment.
Considering that IncidentGenerator encapsulates a database client connection, why not create and get it similarly to get_client(), instead of creating a new one for every object matched?
JenySadadia
force-pushed
the
auto-create-incidents
branch
from
094c755
to
0228687
Compare
Add `tools` directory and add `kcidb_match.py` script there. Co-authored-by: Jeny Sadadia <jeny.sadadia@collabora.com> Signed-off-by: Jeny Sadadia <jeny.sadadia@collabora.com>
Add a subscription module `create_incidents.py` to create incidents automatically when builds and tests objects match with issue patterns. Signed-off-by: Jeny Sadadia <jeny.sadadia@collabora.com>
JenySadadia
force-pushed
the
auto-create-incidents
branch
from
0228687
to
5064d37
Compare
Automate the creation of incidents based on issues using
kcidb_matchtool.