TRT-2381: prereqs for symptoms writing job labels #3174
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
bot
added
the
do-not-merge/hold
openshift-ci-robot
added
the
jira/valid-reference
|
@sosiouxme: This pull request references TRT-2381 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.22.0" version, but no target version was set. DetailsIn 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. |
WalkthroughAdds artifact reader abstraction with gzip handling and context propagation, exposes truncation detection on content matches, migrates job-run annotations to a public JobRunLabel model with JSON-wrapped comments, adds a DB model for job labels, and implements a Sippy HTTP client plus a Symptoms CRUD wrapper. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touchesImportant Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sosiouxme The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@sosiouxme: This pull request references TRT-2381 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.22.0" version, but no target version was set. DetailsIn 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/api/jobartifacts/query.go (1)
182-183: Consider propagating the caller's context instead ofcontext.Background().Using
context.Background()bypasses any timeout or cancellation that might be set by the caller. If the caller's context is cancelled, this operation will continue running.🔎 Proposed fix
The
getFileContentMatchesmethod should accept a context parameter and pass it through:-func (q *JobArtifactQuery) getFileContentMatches(jobRunID int64, attrs *storage.ObjectAttrs) (artifact JobRunArtifact) { +func (q *JobArtifactQuery) getFileContentMatches(ctx context.Context, jobRunID int64, attrs *storage.ObjectAttrs) (artifact JobRunArtifact) { artifact.JobRunID = strconv.FormatInt(jobRunID, 10) artifact.ArtifactPath = relativeArtifactPath(attrs.Name, artifact.JobRunID) artifact.ArtifactContentType = attrs.ContentType artifact.ArtifactURL = fmt.Sprintf(artifactURLFmt, util.GcsBucketRoot, attrs.Name) if q.ContentMatcher == nil { // no matching requested return } - reader, closer, err := OpenArtifactReader(context.Background(), q.GcsBucket.Object(attrs.Name), attrs.ContentType) + reader, closer, err := OpenArtifactReader(ctx, q.GcsBucket.Object(attrs.Name), attrs.ContentType)pkg/db/models/job_labels.go (1)
19-19: Consider usingURLinstead ofUrlfor Go naming conventions.Go conventions prefer
URLas it's an acronym. However, this is a minor style preference.- Url string `bigquery:"-"` + URL string `bigquery:"-"`Note: This would require updating the usage in
jobrunannotator.goline 515 as well.pkg/sippyclient/jobrunscan/symptoms.go (1)
32-40: Consider validating theidparameter.Empty or malformed
idvalues could result in unexpected API paths (e.g.,/api/jobs/symptoms/instead of/api/jobs/symptoms/{id}). While the server should reject these, client-side validation provides earlier, clearer feedback.🔎 Proposed fix
// Get retrieves a single symptom by ID func (sc *SymptomsClient) Get(ctx context.Context, id string) (*jobrunscan.Symptom, error) { + if id == "" { + return nil, fmt.Errorf("symptom id cannot be empty") + } var symptom jobrunscan.Symptom path := fmt.Sprintf("/api/jobs/symptoms/%s", id)Apply similar validation to
UpdateandDeletemethods.pkg/sippyclient/client.go (1)
144-146: Avoid unnecessary byte-to-string conversion.
strings.NewReader(string(bodyBytes))creates an extra string allocation. Usebytes.NewReaderdirectly.🔎 Proposed fix
Add
"bytes"to the imports and update the code:+import ( + "bytes" "context" ... )- bodyReader = strings.NewReader(string(bodyBytes)) + bodyReader = bytes.NewReader(bodyBytes)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
pkg/api/jobartifacts/apitypes.gopkg/api/jobartifacts/query.gopkg/componentreadiness/jobrunannotator/jobrunannotator.gopkg/db/models/job_labels.gopkg/sippyclient/client.gopkg/sippyclient/jobrunscan/symptoms.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/sippyclient/jobrunscan/symptoms.go (1)
pkg/sippyclient/client.go (1)
Client(20-24)
🔇 Additional comments (8)
pkg/api/jobartifacts/apitypes.go (1)
44-48: LGTM!The
HasMatches()helper method is well-implemented with proper nil-safety checks. The comment about future matcher types provides good context for maintainability.pkg/api/jobartifacts/query.go (1)
197-234: LGTM!The
OpenArtifactReaderfunction is well-designed with a robust closer pattern. The nil-safety in the closer function ensures it can always be safely deferred, even when errors occur during initialization. Good use of the transparent gzip handling.pkg/db/models/job_labels.go (1)
8-20: LGTM on the model definition.The
JobRunLabelstruct is well-documented with appropriate BigQuery field mappings. Good use ofbigquery.NullStringfor the optionalUserfield andcivil.DateTimefor timestamp fields.pkg/sippyclient/jobrunscan/symptoms.go (1)
11-21: LGTM on the client structure and constructor.Clean implementation following the functional options pattern from the parent
sippyclient.Client. The composition pattern is appropriate for extending the API surface.pkg/sippyclient/client.go (2)
86-89: Note: Status code handling differs betweenGetanddoJSONRequest.
Getonly accepts200, whiledoJSONRequest(used byPost/Put) accepts the full2xxrange. This is likely intentional since GET requests typically return 200, but worth documenting if this is the expected behavior.
50-64: LGTM on the constructor and options pattern.Clean implementation of the functional options pattern with sensible defaults (30s timeout, localhost for development). The design allows for easy customization via
WithServerURL,WithToken, andWithHTTPClient.pkg/componentreadiness/jobrunannotator/jobrunannotator.go (2)
416-422: LGTM onLabelCommenttype.The versioned approach (
job_run_annotator_v1) is a good practice for schema evolution. Exporting the type enables external tools to parse label comments consistently.
505-516: LGTM on theJobRunLabelconstruction.The new fields (
CreatedAt,UpdatedAt,SourceTool,SymptomID) are properly populated. The comment onSymptomIDclearly indicates future work.
| func (j JobRunAnnotator) generateComment() string { | ||
| comment := j.comment | ||
| annotatorComment, err := json.MarshalIndent(j, "", " ") | ||
| if err == nil { | ||
| comment += fmt.Sprintf("\nAnnotator\n%s", annotatorComment) | ||
| comment := LabelComment{ | ||
| Comment: j.comment, // apparently separated out in order to stand out from the object | ||
| V1: j, | ||
| } | ||
| str, err := json.MarshalIndent(comment, "", " ") | ||
| if err != nil { | ||
| log.WithError(err).Error("error generating JobAnnotator comment") | ||
| } | ||
| return comment | ||
| return string(str) | ||
| } |
There was a problem hiding this comment.
Error in generateComment results in empty string, not propagated.
If json.MarshalIndent fails, the error is logged but an empty string is returned. This could lead to labels being inserted with empty comments, masking the underlying issue.
🔎 Proposed fix
Consider either returning a fallback comment or propagating the error:
func (j JobRunAnnotator) generateComment() string {
comment := LabelComment{
Comment: j.comment,
V1: j,
}
str, err := json.MarshalIndent(comment, "", " ")
if err != nil {
log.WithError(err).Error("error generating JobAnnotator comment")
+ // Return a fallback comment with the original prefix
+ return j.comment
}
return string(str)
}Alternatively, if this should never fail, consider using panic similar to the pattern in CacheKeyForJobRun (line 278 in query.go).
🤖 Prompt for AI Agents
In pkg/componentreadiness/jobrunannotator/jobrunannotator.go around lines 424 to
434, generateComment currently logs json.MarshalIndent errors and returns an
empty string which masks failures; change generateComment to return (string,
error) instead of string, call json.MarshalIndent and on error return "", err
(or if you intentionally want a hard failure, replace the log+return with a
panic consistent with CacheKeyForJobRun), then update all call sites to handle
the error (propagate or handle fallback) and add/update tests to cover marshal
failure behavior.
sosiouxme
force-pushed
the
20251216-label-symptoms-CF
branch
from
c0e3183 to
dc2bf48
Compare
|
@sosiouxme: This pull request references TRT-2381 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.22.0" version, but no target version was set. DetailsIn 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/sippyclient/client.go (1)
68-97: Consider nil-check for consistency with doJSONRequest.Line 92 decodes the response without checking if
resultis nil, whiledoJSONRequest(line 171) does perform this check. For consistency and defensive programming, consider adding a nil check here as well.🔎 Optional defensive nil check
} + if result != nil { if err := json.NewDecoder(resp.Body).Decode(result); err != nil { return fmt.Errorf("failed to decode response: %w", err) } + } return nil
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (8)
pkg/api/jobartifacts/apitypes.gopkg/api/jobartifacts/manager.gopkg/api/jobartifacts/query.gopkg/api/jobartifacts/query_test.gopkg/componentreadiness/jobrunannotator/jobrunannotator.gopkg/db/models/job_labels.gopkg/sippyclient/client.gopkg/sippyclient/jobrunscan/symptoms.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/api/jobartifacts/apitypes.go
- pkg/db/models/job_labels.go
🧰 Additional context used
🧬 Code graph analysis (4)
pkg/api/jobartifacts/query.go (1)
pkg/api/jobartifacts/apitypes.go (2)
JobRunArtifact(29-37)MatchedContent(39-42)
pkg/sippyclient/jobrunscan/symptoms.go (1)
pkg/sippyclient/client.go (1)
Client(21-25)
pkg/componentreadiness/jobrunannotator/jobrunannotator.go (2)
pkg/db/models/job_labels.go (1)
JobRunLabel(8-19)vendor/github.com/invopop/jsonschema/id.go (1)
ID(12-12)
pkg/api/jobartifacts/query_test.go (3)
pkg/api/jobartifacts/content_line_matcher.go (1)
NewStringMatcher(74-81)pkg/api/jobartifacts/apitypes.go (2)
MatchedContent(39-42)ContentLineMatches(50-54)pkg/api/jobartifacts/query.go (1)
ContentMatcher(237-246)
🔇 Additional comments (20)
pkg/api/jobartifacts/query_test.go (1)
50-69: LGTM! Context propagation properly implemented in tests.The test updates correctly pass
context.Contextas the first parameter togetFileContentMatches, aligning with the new signature. The use ofcontext.Background()is appropriate for test scenarios.pkg/api/jobartifacts/manager.go (1)
269-269: LGTM! Proper context propagation for artifact queries.The updated call correctly passes
request.ctxtogetFileContentMatches, enabling proper cancellation and timeout handling during artifact content matching. This aligns with theartifactQueryTimeoutconstraint defined at line 22.pkg/api/jobartifacts/query.go (2)
173-195: LGTM! Well-designed refactoring with proper context propagation.The refactored
getFileContentMatchesfunction correctly:
- Accepts context for cancellation/timeout support
- Uses the new
OpenArtifactReaderabstraction to handle gzip content transparently- Properly manages resources via the closer function with
defer- Preserves incomplete matches even when scanning encounters errors (line 193), which is valuable for debugging
The explicit field assignments from
attrs(lines 175-177) improve clarity.
197-234: LGTM! Excellent resource management pattern.The
OpenArtifactReaderfunction is well-designed:
- Closer pattern: Always returns a non-nil closer function that's safe to defer, even on error. The closer checks for nil before closing each reader (lines 206-211), preventing panics.
- Transparent gzip handling: Automatically detects and decompresses gzipped content based on
contentType(line 221).- Context propagation: Passes context to
NewReader(line 215) for proper cancellation support.- Defensive nil assignments (lines 217, 225): Explicit nil assignments before returning on error make the intent clear and ensure the closer behaves correctly, even though these assignments are technically redundant given Go's error conventions.
pkg/componentreadiness/jobrunannotator/jobrunannotator.go (5)
416-422: LGTM! Well-designed extensible comment structure.The exported
LabelCommenttype provides a clean, extensible way to serialize tool-specific context as JSON. The design allows different tools to add their own keys while preserving a commoncommentfield.
424-436: LGTM! Error handling improved with fallback comment.The function now handles
json.MarshalIndenterrors by returning a fallback comment that includes the error message, rather than an empty string. This ensures failed serialization is visible in the DB and provides debugging information.This pragmatically addresses the past review concern about masking marshal failures.
438-491: LGTM! Consistent migration to public model.The function signature and implementation correctly migrated to use
models.JobRunLabelthroughout. The BigQuery query and row iteration are properly updated.Note: Consistent with the model definition, the
URLfield won't be populated from BigQuery due to itsbigquery:"-"tag.
493-523: LGTM! Properly populates all JobRunLabel fields.The function correctly constructs
models.JobRunLabelinstances with:
- Required fields mapped from input (
ID,StartTime,Label,User,URL)- Metadata fields properly initialized (
CreatedAt,UpdatedAt,SourceTool)- JSON-wrapped comment via
generateComment()- Intentionally empty
SymptomIDwith clear documentationThe hardcoded
SourceToolvalue is appropriate for this specific annotation tool.
387-393: URL field withbigquery:"-"tag is intentional; no action needed.The
models.JobRunLabel.URLfield correctly has thebigquery:"-"tag and is not stored in BigQuery. This is intentional design—the field is populated for dry-run display (line 393) and in-memory use, but persistent storage is not required since the job can be looked up by its ID (prowjob_build_id). The table schema confirms URL is not among the persisted fields.pkg/sippyclient/client.go (6)
14-25: LGTM! Clean client structure.The Client struct and default constants are well-designed. The comment explaining the intended usage (cloud functions, CLI tools) provides helpful context for understanding why this code may not be used within Sippy itself.
27-49: LGTM! Idiomatic functional options pattern.The functional options are well-implemented. Line 33's
strings.TrimSuffixis particularly good practice to ensure clean URL construction.
52-65: LGTM! Well-structured constructor.The constructor properly initializes defaults and applies options. The 30-second timeout is a sensible default for API calls.
100-107: LGTM! Clean delegation pattern.Post and Put methods correctly delegate to the shared
doJSONRequesthelper.
110-134: LGTM! Proper DELETE semantics.The method correctly handles both
StatusOK(200) andStatusNoContent(204) responses, which are both valid for DELETE operations.
137-178: LGTM! Robust JSON request handler.The method properly handles all aspects of JSON requests:
- Marshals request body only when present
- Sets appropriate headers
- Checks for all 2xx success codes (appropriate for POST/PUT)
- Defensively checks if result is nil before decoding
pkg/sippyclient/jobrunscan/symptoms.go (5)
12-22: LGTM! Clean wrapper design.The SymptomsClient properly wraps the generic HTTP client, providing good separation between HTTP mechanics and domain-specific API logic.
24-31: LGTM! Straightforward List implementation.The method correctly retrieves all symptoms and wraps errors appropriately.
49-56: LGTM! Clean Create implementation.The method correctly creates a new symptom and returns the server's response.
58-70: LGTM! Correct Update implementation.The method properly validates the ID and updates the symptom.
72-83: LGTM! Proper Delete implementation.The method correctly validates the ID and performs the deletion.
| var idSanityRegex = regexp.MustCompile(`^\w+$`) // slightly permissive but close enough to be safe | ||
|
|
||
| // Get retrieves a single symptom by ID | ||
| func (sc *SymptomsClient) Get(ctx context.Context, id string) (*jobrunscan.Symptom, error) { | ||
| if !idSanityRegex.MatchString(id) { | ||
| return nil, fmt.Errorf("invalid symptom ID: '%s'", id) | ||
| } | ||
|
|
||
| var symptom jobrunscan.Symptom | ||
| path := fmt.Sprintf("/api/jobs/symptoms/%s", id) | ||
| if err := sc.client.Get(ctx, path, &symptom); err != nil { | ||
| return nil, fmt.Errorf("failed to get symptom %s: %w", id, err) | ||
| } | ||
| return &symptom, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find examples of symptom IDs and the Symptom struct definition
echo "=== Searching for symptom ID patterns ==="
rg -n -C3 'symptom.*[Ii][Dd]|Symptom.*ID' --type=go 2>/dev/null | head -50
echo ""
echo "=== Searching for Symptom struct definition ==="
ast-grep --pattern 'type Symptom struct {
$$$
}'
echo ""
echo "=== Checking for UUID patterns in symptoms context ==="
rg -n 'Symptom|symptom' --type=go | rg -i 'uuid|guid|[0-9a-f]{8}-' | head -20
echo ""
echo "=== Looking for test files related to symptoms ==="
fd -e go --path '*symptom*' --path '*test*'Repository: openshift/sippy
Length of output: 6326
🏁 Script executed:
#!/bin/bash
echo "=== Looking for test files and example symptom IDs ==="
fd -type f -name '*symptom*.go' | head -20
echo ""
echo "=== Searching for symptom creation/test examples ==="
rg -n -A5 'Symptom\{' --type=go | grep -E '(ID|Test|func)' | head -40
echo ""
echo "=== Checking server-side symptom validation ==="
rg -n 'idSanityRegex|validateSymptom|symptom.*ID' --type=go -B2 -A2 | grep -v Binary
echo ""
echo "=== Looking for actual ID constraint enforcement ==="
rg -n 'not.*start|digit|[Ii]dentifier.*valid' --type=go | head -30Repository: openshift/sippy
Length of output: 10968
🏁 Script executed:
#!/bin/bash
echo "=== Finding validIdentifierRegex definition ==="
rg -n 'validIdentifierRegex\s*=' --type=go -A1
echo ""
echo "=== Full context of validateSymptom function ==="
rg -n 'func validateSymptom' --type=go -A15
echo ""
echo "=== Check the full pkg/api/jobrunscan/symptoms.go file ==="
head -50 pkg/api/jobrunscan/symptoms.goRepository: openshift/sippy
Length of output: 3003
Fix client-side ID validation to match server-side constraints.
The idSanityRegex pattern (^\w+$) is more permissive than the server-side validation in pkg/api/jobrunscan/helpers.go, which requires identifiers to start with a letter or underscore (^[a-zA-Z_][a-zA-Z0-9_]*$). This creates a validation mismatch where the client accepts IDs that the server will reject. For example, an ID starting with a digit like "123test" would pass client validation but fail server validation. Update the regex to ^\[a-zA-Z_\][a-zA-Z0-9_]*$ to ensure consistency.
🤖 Prompt for AI Agents
In pkg/sippyclient/jobrunscan/symptoms.go around lines 33 to 47, the client-side
ID validation uses idSanityRegex = `^\w+$` which is more permissive than the
server's rule and allows IDs starting with digits; update the regex to enforce
the same constraint as the server by changing it to `^[a-zA-Z_][a-zA-Z0-9_]*$`
so IDs must start with a letter or underscore and contain only letters, digits,
or underscores, ensuring client and server validation match.
There was a problem hiding this comment.
this is a sanity check, not a validity check. your suggestion was just to check that it not be empty. this also checks that it's not something broken or even maliciously inserted.
There was a problem hiding this comment.
@sosiouxme Thank you for clarifying the intent! You're absolutely right—a client-side sanity check that prevents obviously malformed or malicious input (like injection attempts or path traversal) while letting the server enforce the precise business rules is a sound architectural pattern. The looser ^\w+$ pattern still blocks special characters and suspicious patterns while allowing the server to be the authoritative validator.
I understand now that this is an intentional design decision, not an oversight. The server-side validation will catch any IDs that don't meet the stricter format requirements.
🤖 Assisted by Claude Code
Label comments should be json with a tool-specific schema.
This was a suggestion from CodeRabbit that seemed to make sense. If the request context gets canceled, then the results are going to be abandoned anyway, so it makes sense to let that cancellation pass to the GCS reader and give it a chance to abort. It seems right... It should be a minor optimization... But check if anything breaks.
sosiouxme
force-pushed
the
20251216-label-symptoms-CF
branch
from
dc2bf48 to
c1d7597
Compare
|
@sosiouxme: This pull request references TRT-2381 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.22.0" version, but no target version was set. DetailsIn 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. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/sippyclient/jobrunscan/symptoms.go (1)
33-47: Client-side ID validation is more permissive than server-side (previously flagged).The regex
^\w+$allows IDs starting with digits, but the server requires IDs to match^[a-zA-Z_][a-zA-Z0-9_]*$. This was already identified in a previous review.
🧹 Nitpick comments (3)
pkg/sippyclient/client.go (2)
87-90: Consider accepting 2xx range for consistency withdoJSONRequest.The
Getmethod only accepts200 OK, whiledoJSONRequest(used by Post/Put) accepts any 2xx status. This inconsistency could cause issues if the API returns204 No Contentor other valid 2xx responses for GET requests.🔎 Proposed fix
- if resp.StatusCode != http.StatusOK { + if resp.StatusCode < 200 || resp.StatusCode >= 300 { body, _ := io.ReadAll(resp.Body) return fmt.Errorf("unexpected status code %d: %s", resp.StatusCode, string(body)) }
166-168: Variable shadowing:bodyshadows the function parameter.The
bodyvariable declared here shadows thebodyparameter on line 137. While this works, it can be confusing. Consider using a different name likerespBody.🔎 Proposed fix
if resp.StatusCode < 200 || resp.StatusCode >= 300 { - body, _ := io.ReadAll(resp.Body) - return fmt.Errorf("unexpected status code %d: %s", resp.StatusCode, string(body)) + respBody, _ := io.ReadAll(resp.Body) + return fmt.Errorf("unexpected status code %d: %s", resp.StatusCode, string(respBody)) }pkg/sippyclient/jobrunscan/symptoms.go (1)
49-56: Consider validating symptom.ID in Create method.Unlike
Get,Update, andDelete, theCreatemethod doesn't validate the symptom ID before sending the request. If the symptom struct contains an invalid ID, the error will only be caught server-side.🔎 Proposed fix
func (sc *SymptomsClient) Create(ctx context.Context, symptom jobrunscan.Symptom) (*jobrunscan.Symptom, error) { + if symptom.ID != "" && !idSanityRegex.MatchString(symptom.ID) { + return nil, fmt.Errorf("invalid symptom ID: '%s'", symptom.ID) + } var result jobrunscan.Symptom if err := sc.client.Post(ctx, "/api/jobs/symptoms", symptom, &result); err != nil { return nil, fmt.Errorf("failed to create symptom: %w", err) } return &result, nil }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (8)
pkg/api/jobartifacts/apitypes.gopkg/api/jobartifacts/manager.gopkg/api/jobartifacts/query.gopkg/api/jobartifacts/query_test.gopkg/componentreadiness/jobrunannotator/jobrunannotator.gopkg/db/models/job_labels.gopkg/sippyclient/client.gopkg/sippyclient/jobrunscan/symptoms.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/api/jobartifacts/query_test.go (3)
pkg/api/jobartifacts/content_line_matcher.go (1)
NewStringMatcher(74-81)pkg/api/jobartifacts/apitypes.go (2)
MatchedContent(39-42)ContentLineMatches(50-54)pkg/api/jobartifacts/query.go (1)
ContentMatcher(237-246)
pkg/componentreadiness/jobrunannotator/jobrunannotator.go (1)
pkg/db/models/job_labels.go (1)
JobRunLabel(8-19)
pkg/sippyclient/jobrunscan/symptoms.go (1)
pkg/sippyclient/client.go (1)
Client(21-25)
🔇 Additional comments (14)
pkg/db/models/job_labels.go (2)
7-19: LGTM! Well-structured BigQuery model.The
JobRunLabelstruct correctly maps to BigQuery with appropriate field types. Good use ofcivil.DateTimefor timestamp fields and thebigquery:"-"tag for the URL field that's only needed in-memory.
21-32: Schema update documentation is helpful.Including the
bq updateand SQLALTER TABLEcommands directly in the source code is a practical approach for coordinating schema changes with code deployments. Consider also documenting that these columns should be nullable since existing rows won't have values.pkg/sippyclient/client.go (1)
51-65: LGTM! Clean functional options pattern.The constructor with functional options is idiomatic and provides sensible defaults. The 30-second timeout is reasonable for API calls.
pkg/sippyclient/jobrunscan/symptoms.go (1)
12-22: LGTM! Clean client wrapper.The
SymptomsClientproperly encapsulates the base client and provides a focused API for symptoms operations.pkg/componentreadiness/jobrunannotator/jobrunannotator.go (4)
416-436: Good improvement: generateComment now has a sensible fallback.The previous review flagged that
generateCommentreturned an empty string on error. This has been addressed with a fallback that includes the original comment and error message, making debugging easier while ensuring labels are never inserted with empty comments.
387-414: LGTM! Clean migration to JobRunLabel model.The
bulkInsertJobRunAnnotationscorrectly uses the newmodels.JobRunLabeltype and properly accesses the capitalizedURLfield.
438-491: LGTM! BigQuery fetch correctly updated for new model.The
getJobRunAnnotationsFromBigQueryproperly returnsmap[int64]models.JobRunLabeland decodes rows into the new struct type.
493-523: LGTM! annotateJobRuns properly populates all new fields.Good use of
civil.DateTimeOf(time.Now())for timestamps, and the SourceTool is set to a meaningful identifier. The empty SymptomID for manual annotations is clearly documented.pkg/api/jobartifacts/query_test.go (2)
48-63: LGTM! Tests properly updated for context-aware API.The test correctly creates a context and passes it to
getFileContentMatches, aligning with the new function signature.
65-76: LGTM! Gzip test updated with context.The gzip content test properly passes
context.Background()to the updated API.pkg/api/jobartifacts/apitypes.go (1)
44-48: LGTM! Clean helper method for match detection.The
HasMatches()method provides a nil-safe way to check for matches. The comment acknowledging future complexity is helpful for maintainability.pkg/api/jobartifacts/manager.go (1)
265-279: LGTM! Context properly propagated to content matching.The worker now passes the request context to
getFileContentMatches, enabling proper cancellation and timeout handling for artifact content scanning.pkg/api/jobartifacts/query.go (2)
173-195: LGTM! Clean refactor with proper error handling.The
getFileContentMatchesfunction now properly usesOpenArtifactReaderfor transparent gzip handling. Good decision to still return partial matches even when scanning encounters an error - this aids debugging.
197-234: Well-designed resource cleanup pattern in OpenArtifactReader.The closure-based cleanup approach is elegant and handles all resource cleanup scenarios correctly:
- Returns closer before error so callers can always defer it
- Nil-checks in closer prevent issues when readers aren't initialized
- Setting readers to nil on error prevents unnecessary close attempts
The gzip content type check correctly handles the standard
application/gzipMIME type as provided by the GCS API.
|
Scheduling required tests: |
|
@sosiouxme: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
2 participants
This prepares sippy for the work being done in the cloud function to begin applying labels automatically. Some is to make sippy's code reusable in that function, some is just to adjust to the updated design for labels.
Individual commits represent bite-sized concepts for review.
/hold
This requires an accompanying schema update which should be performed once approved for merge.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.