Forbid failing incidents from being scheduled in aggregates #154
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #154 +/- ##
==========================================
+ Coverage 66.84% 67.49% +0.64%
==========================================
Files 24 25 +1
Lines 1659 1692 +33
==========================================
+ Hits 1109 1142 +33
Misses 550 550 ☔ View full report in Codecov by Sentry. |
foursixnine
force-pushed
the
veteolvidamicaramicasaminombreypegalavuelta
branch
2 times, most recently
from
0d80e14 to
851afb1
Compare
|
@okurz since you're requesting changes already, can you guide me in the test/code paths? |
|
@okurz can you please tag this pr as |
|
We don't have that kind of label yet (in that repo). Are you saying you've been using AI here? If yes, why is that relevant and what would adding that tag change? |
It add traceability, of things that have been done using some sort of assistance, while not important for you, SUSE-wise it is. |
foursixnine
force-pushed
the
veteolvidamicaramicasaminombreypegalavuelta
branch
from
51e7792 to
091b1cf
Compare
As I explained I don't think it's a good idea but as you insist I created and added that label. |
foursixnine
force-pushed
the
veteolvidamicaramicasaminombreypegalavuelta
branch
from
ccb9ef0 to
5622827
Compare
|
This pull request is now in conflicts. Could you fix it? 🙏 |
foursixnine
force-pushed
the
veteolvidamicaramicasaminombreypegalavuelta
branch
from
8f2a737 to
d436336
Compare
Makefile
Outdated
| # devel: environment | ||
| # maybe use Makefile.VENV instead to get a shell with virtualenv | ||
| # # we need to detect what shell we are using | ||
| # shell=$$(basename $$SHELL); \ | ||
| # echo "Activating virtualenv for $$shell"; \ | ||
| # . $(VENV) && \ | ||
| # exec $($(SHELL)) |
There was a problem hiding this comment.
Maybe keep it on a different branch for now?
There was a problem hiding this comment.
Not really, updated the comment though; Not adding another TODO, to avoid causing a heart attack to @okurz
There was a problem hiding this comment.
Actually this might be worse than a TODO as it's dead/disabled code and nobody will know why it's not enabled. I recommend you just remove that
There was a problem hiding this comment.
@okurz thank you for the recommendation, however same comment applies:
There was a problem hiding this comment.
It would make sense to at least state why this code has been disabled; e.g. why it is not good/useful enough for general use and in what situations it would make sense to use it nevertheless. The comments
# Developers have bad memory, so we need to remind them to activate the virtualenv
# maybe use Makefile.VENV instead to get a shell with virtualenv
don't make that clear to me at all.
Additionally, also if that code was not commented-out I'd frankly struggle to make sense of its intended use and purpose. So that should probably be clarified anyway.
There was a problem hiding this comment.
Additionally, also if that code was not commented-out I'd frankly struggle to make sense of its intended use and purpose. So that should probably be clarified anyway.
Good point, updated the comment
foursixnine
force-pushed
the
veteolvidamicaramicasaminombreypegalavuelta
branch
2 times, most recently
from
ffb63a0 to
905101f
Compare
tests/test_incident.py
Outdated
| {"status": "passed", "job_id": 1}, | ||
| {"status": "failed", "job_id": 1777}, # Accept the turk | ||
| { | ||
| "status": "softfailed", | ||
| "job_id": 2020, | ||
| }, # 2020 is the genesys of dark fate | ||
| {"status": "failed", "job_id": 2042}, # This one has a dark fate | ||
| {"status": "passed", "job_id": 3}, |
There was a problem hiding this comment.
Python linting can be ugly at times 🗡️
foursixnine
force-pushed
the
veteolvidamicaramicasaminombreypegalavuelta
branch
from
905101f to
b194cd7
Compare
| # - remove almost duplicated code from Approver.is_job_marked_acceptable_for_incident | ||
| # as approver does not seem to operate over incidents | ||
| # about the TODO see discussion at https://github.com/openSUSE/qem-bot/pull/154#discussion_r1472721681 | ||
| @staticmethod |
There was a problem hiding this comment.
Of class Incident is suppose. I'm not that familiar with Python so I'm wondering what are you getting at. Can you provide a concrete suggestion?
There was a problem hiding this comment.
OF which class? look at place whole has_ignored_comment is a function not a method .. and isn't part of any class
In perl class is usualy whole file , In python identaton and place matter :D
| ) | ||
|
|
||
| if not results: | ||
| raise NoResultsError( |
There was a problem hiding this comment.
is this exception anywhere caught? and resolved?, btw aggregates could be scheduled before any results are available ..
|
This pull request is now in conflicts. Could you fix it? 🙏 |
The code used by approver doesn't seem to use Incidents class and a rewrite at this point has less benefit than simply extracting the duplicated regular expression. A TODO has been left in place to keep track for subsequent PRs steming from discussion in [1] [1] openSUSE#154 (comment)
foursixnine
force-pushed
the
veteolvidamicaramicasaminombreypegalavuelta
branch
from
e551352 to
5c9d366
Compare
|
@okurz as agreed last week, here is the Pull request to deploy Forbid failing incidents from being scheduled in aggregates #154 which would have helped with the sudo update of yesterday |
| # - remove almost duplicated code from Approver.is_job_marked_acceptable_for_incident | ||
| # as approver does not seem to operate over incidents | ||
| # about the TODO see discussion at https://github.com/openSUSE/qem-bot/pull/154#discussion_r1472721681 | ||
| @staticmethod |
There was a problem hiding this comment.
Of class Incident is suppose. I'm not that familiar with Python so I'm wondering what are you getting at. Can you provide a concrete suggestion?
| # TODO: | ||
| # - move to utils.py or a better place | ||
| # - remove almost duplicated code from Approver.is_job_marked_acceptable_for_incident |
There was a problem hiding this comment.
These two points don't seem to hard to implement. Am I overlooking something or can we maybe just do them before merging this PR?
There was a problem hiding this comment.
can we maybe just do them before merging this PR?
I'll leave it for a follow-up when addressing the rest of the changes, as that would need a bigger refactor, due to the incidents class not being used in the approver. thingie.
| for comment in ret: | ||
| if regex.match(comment["text"]): | ||
| # leave comment for future debugging purposes | ||
| # log.debug("matched comment incident %s: with comment %s", inc, comment) |
There was a problem hiding this comment.
In fact, after last changes, this is not necessary anymore so I dropped them
It looks like the current version on GitHub still has the disabled line.
|
@Mergifyio rebase |
Let the build system die if any errors are found, this is intended for local development only.
Leave early when filtering incidents to schedule, as incidents that have failures don't need further processing. Adjust tests accordingly fixing that ugly off by one
- While incidents are less likely to have an exception comment, there are cases where a failing aggregate from day before, might impact an incident on its own to be scheduled - We want to accept only passed results without questioning, anything else, will need to have an acceptable_for, following the discussion in [1] [1] https://github.com/openSUSE/qem-bot/pull/154/files#r1474042954
The code used by approver doesn't seem to use Incidents class and a rewrite at this point has less benefit than simply extracting the duplicated regular expression. A TODO has been left in place to keep track for subsequent PRs steming from discussion in [1] [1] openSUSE#154 (comment)
qem-bot's data is normalized, so either passed or failed.
✅ Branch has been successfully rebased |
Martchus
force-pushed
the
veteolvidamicaramicasaminombreypegalavuelta
branch
from
5c9d366 to
36ed6db
Compare
|
I also called the application locally but found no relevant logs are output: Possibly the relevant steps are not executed due to dry-run or something else preventing the evaluation of what products to trigger: EDIT: I also called and compared both output logs to see if there is any reasonable difference. "inc-sync-results" provide a way too huge list of results to process, inc-approve shows a lot of difference but due to changed realtime results, not related to this pull request. The relevant commands (if at all) are "full-run incidents-run updates-run" and there are no differences at all in the output (except for timestamps) meaning what I stated in before: Possibly the relevant steps are not executed due to dry-run or something else preventing the evaluation of what products to trigger and further more significant reverse-engineering would be necessary to change that. |
|
This pull request is now in conflicts. Could you fix it? 🙏 |
Sparked from discussion in: openSUSE/qem-bot#154 (review)
@mimi1vx, @michaelgrifalconi