TRT-1702: record risk analysis results in BQ #28926
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sosiouxme: This pull request references TRT-1702 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@sosiouxme: This pull request references TRT-1702 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| endTime := time.Now() | ||
| duration := endTime.Sub(startTime) | ||
| logrus.Infof("Call to sippy finished after: %s", duration) | ||
| cancelFn() // cancel the context timeout we just used. |
There was a problem hiding this comment.
Looks like you call this via defer on line 189, is it intentional to call it again?
There was a problem hiding this comment.
Ok, looks like the same call was previously at line 121.
There was a problem hiding this comment.
it's how it was when i started, just moved slightly. i wasn't sure i understood the context well enough to change. i gather it's to clean up the timeout, and if it hasn't already timed out, keep it from firing in the middle of the rest of the code (but what would happen if it did?). the defer does seem superfluous as there's no way it could make it through an iteration without calling cancelFn() but... calling it twice seems unlikely to cause problems?
There was a problem hiding this comment.
Yep caught that after my first comment. It is how it was so let's leave it.
pkg/riskanalysis/cmd.go
Outdated
| Rows: []map[string]string{}, | ||
| } | ||
| dataFile.Rows = append(dataFile.Rows, map[string]string{ | ||
| "TestName": "", // no test name means overall risk |
There was a problem hiding this comment.
Functionally it is probably all the same but wondering if we should explicitly say 'Overall' here?
There was a problem hiding this comment.
I think it's a matter of taste, as long as you're confident there will never be a test named "Overall". I was considering separating these out with another boolean column, or even in a separate table from regular test results. I am open to any of these. To me, if there's to be a special value that says "not an individual test", it makes sense for it to be the empty string.
There was a problem hiding this comment.
I ended up separating these into two tables after all, since they had different enriched content.
sosiouxme
force-pushed
the
20240705-ra-recording
branch
from
d30f540
to
7ee51c3
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neisw, sosiouxme The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
sosiouxme
added
the
acknowledge-critical-fixes-only
|
@sosiouxme: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/hold Revision 7ee51c3 was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
|
Job Failure Risk Analysis for sha: 7ee51c3
|
|
not sure what the bot is smoking, since blocking tests all passed. looks like it's trying to retest but there are none required? anyway i don't see a need for the hold. the RA looks scary but there's a lot going on and these can't possibly be related to my change. /hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
hm. yeah. the bots seem to have mixed ideas about what tests are required. |
openshift-merge-bot
bot
merged commit 946048d
into
openshift:master
|
[ART PR BUILD NOTIFIER] Distgit: openshift-enterprise-tests |
This hooks into the existing RA process to write autodl artifacts resulting in both a log of any attempts to request RA (and how they went) in one table as well as another table with the results themselves from RA for later aggregated analysis.