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 MatchedContent.HasMatches and ContentLineMatches.Truncated; introduces a gzip-aware OpenArtifactReader and propagates context into artifact matching; migrates job-run annotations to a new public models.JobRunLabel with JSON-wrapped comments; adds a Sippy HTTP client and a Symptoms CRUD wrapper; exports a jobrunscan identifier regex. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing touches
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.
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.
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
force-pushed
the
20251216-label-symptoms-CF
branch
from
c1d7597 to
66f527a
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: 2
🧹 Nitpick comments (1)
pkg/api/jobartifacts/query.go (1)
200-212: Consider whether ignoring close errors is acceptable.The closer function discards errors from
gzipReader.Close()andgcsReader.Close(). While this is common practice for read-only streams (since we've already consumed the data), it may hide issues during debugging. If you want visibility into close failures, consider logging at debug level.That said, for a read-only artifact retrieval flow, this is generally acceptable.
📜 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 (11)
pkg/api/jobartifacts/apitypes.gopkg/api/jobartifacts/manager.gopkg/api/jobartifacts/query.gopkg/api/jobartifacts/query_test.gopkg/api/jobrunscan/helpers.gopkg/api/jobrunscan/labels.gopkg/api/jobrunscan/symptoms.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 (4)
- pkg/api/jobartifacts/apitypes.go
- pkg/api/jobartifacts/manager.go
- pkg/db/models/job_labels.go
- pkg/sippyclient/client.go
🧰 Additional context used
🧬 Code graph analysis (2)
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/api/jobrunscan/symptoms.go (2)
pkg/api/jobrunscan/helpers.go (1)
ValidIdentifierRegex(13-13)vendor/github.com/invopop/jsonschema/id.go (1)
ID(12-12)
🔇 Additional comments (21)
pkg/componentreadiness/jobrunannotator/jobrunannotator.go (5)
21-21: LGTM!Import of the models package enables the migration to the public
JobRunLabeltype as intended.
416-422: LGTM!The
LabelCommentstructure provides good separation between human-readable comments and versioned machine-readable context, allowing for future schema evolution.
438-438: Type migration tomodels.JobRunLabelin read path.The function now returns and processes
models.JobRunLabelinstead of the private type. This is part of the migration to public types, and BigQuery will unmarshal rows using struct tags onmodels.JobRunLabel.Refer to the verification requested on Line 387 to ensure the struct has compatible BigQuery tags.
Also applies to: 451-451, 461-461
500-500: LGTM!The struct construction properly populates all fields including the new metadata:
CreatedAt/UpdatedAtset to current timeSourceToolidentifies the annotation sourceSymptomIDintentionally empty for manual annotationsThe implementation correctly prepares labels for insertion with rich metadata as intended by the design.
Also applies to: 507-518
387-387: models.JobRunLabel has proper BigQuery schema tags.The struct has all required bigquery tags matching the job_labels table schema (prowjob_build_id, prowjob_start, label, comment, user, created_at, updated_at, source_tool, symptom_id). The URL field is correctly tagged with
bigquery:"-"to exclude it from insertion, which is intentional—it's populated at runtime for logging purposes only at line 393.pkg/api/jobartifacts/query_test.go (3)
50-53: LGTM!The context is properly initialized and passed to
getFileContentMatches, aligning with the updated signature that now requires context propagation for cancellation/timeout support.
58-59: LGTM!The reuse of the same
ctxvariable for the second call is appropriate since both calls are within the same test and share the same lifecycle.
65-69: LGTM!The test correctly passes
context.Background()along with the gzip content type, properly exercising the new gzip-aware reader functionality introduced inOpenArtifactReader.pkg/api/jobartifacts/query.go (5)
33-33: LGTM!The TODO comment appropriately documents planned future enhancements for content matching capabilities.
173-195: LGTM!The updated function properly:
- Propagates context for cancellation/timeout support
- Uses the new
OpenArtifactReaderabstraction for transparent gzip handling- Places
defer closer()before the error check, which is correct sinceOpenArtifactReaderguarantees a safe-to-call closer regardless of error state- Preserves incomplete matches even when scanning encounters an error (useful for debugging partial results)
215-219: LGTM on the nil-assignment pattern for cleanup safety.Setting
gcsReader = nilafter a failedNewReadercall ensures the closer won't attempt to close an invalid reader. This is a good defensive pattern.
197-234: Well-designed reader abstraction.The
OpenArtifactReaderfunction cleanly encapsulates:
- Context-aware GCS reader creation
- Transparent gzip decompression
- A safe-to-call closer function regardless of error state
This design allows callers to unconditionally
defer closer()without worrying about nil checks or partial initialization states.
221-228: Consider checking ContentEncoding instead of ContentType for gzip detection.The code checks
contentType == "application/gzip", but according to GCS and HTTP standards, file compression is indicated via theContentEncodingfield, notContentType. Prow's compression configuration explicitly states: "the content-encoding on these files will be set to gzip." The current implementation should verifyattrs.ContentEncoding == "gzip"instead of relying on a specific ContentType value, as ContentType represents the MIME type of the underlying content (e.g., "application/json", "text/plain").pkg/sippyclient/jobrunscan/symptoms.go (4)
17-31: LGTM!The constructor and List method are cleanly implemented with proper context propagation and error wrapping.
33-45: LGTM!The Get method properly validates the ID and handles errors. The client-side validation serves as a sanity check while the server performs authoritative validation, which is a sound architectural pattern.
47-58: LGTM!The Create method validates the symptom ID and properly handles the API interaction with appropriate error context.
74-85: LGTM!The Delete method properly validates the ID before deletion and includes appropriate error context.
pkg/api/jobrunscan/symptoms.go (1)
17-17: LGTM!The change to export
ValidIdentifierRegexmakes the validation pattern reusable across packages, supporting the client implementation while maintaining the same validation logic.pkg/api/jobrunscan/helpers.go (2)
13-13: LGTM!Exporting
ValidIdentifierRegexenables consistent validation across both the API and client packages, which aligns with the PR's goal of making sippy code reusable for the cloud function.
18-62: LGTM!The
generateUniqueIDFromTitlefunction is well-implemented with proper sanitization, collision handling, and security considerations (usingregexp.QuoteMetato prevent injection).pkg/api/jobrunscan/labels.go (1)
21-21: LGTM!The change to use the exported
ValidIdentifierRegexmaintains consistency with the symptoms validation and supports code reuse across the package.
|
Scheduling required tests: |
Also, use it to implement such a client for symptoms. 🤖 Assisted by Claude Code
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.
Label comments should be json with a tool-specific schema.
sosiouxme
force-pushed
the
20251216-label-symptoms-CF
branch
from
66f527a to
f487f04
Compare
| // [2025-12-22] To update the BigQuery table schema, use: | ||
| // | ||
| // bq update <project>:<dataset>.job_labels \ | ||
| // created_at:DATETIME,updated_at:DATETIME,source_tool:STRING,symptom_id:STRING | ||
| // | ||
| // Or via SQL: | ||
| // | ||
| // ALTER TABLE `<project>.<dataset>.job_labels` | ||
| // ADD COLUMN IF NOT EXISTS created_at DATETIME, | ||
| // ADD COLUMN IF NOT EXISTS updated_at DATETIME, | ||
| // ADD COLUMN IF NOT EXISTS source_tool STRING, | ||
| // ADD COLUMN IF NOT EXISTS symptom_id STRING; |
There was a problem hiding this comment.
use this to update the schema before this merges.
just in case, best to make a copy of the table first.
|
@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
♻️ Duplicate comments (1)
pkg/componentreadiness/jobrunannotator/jobrunannotator.go (1)
416-436: Structured comment design is sound; fallback remains non-JSON.The
LabelCommenttype provides a clean JSON structure for wrapping annotation context. The fallback on line 433 still produces non-JSON output, which was flagged in a previous review and accepted as a minor issue given that marshaling this struct should rarely fail in practice.Systems consuming these comments should handle the non-JSON fallback gracefully, or consider implementing the suggestion from the previous review to return valid JSON even in the error case.
📜 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 (11)
pkg/api/jobartifacts/apitypes.gopkg/api/jobartifacts/manager.gopkg/api/jobartifacts/query.gopkg/api/jobartifacts/query_test.gopkg/api/jobrunscan/helpers.gopkg/api/jobrunscan/labels.gopkg/api/jobrunscan/symptoms.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 (5)
- pkg/api/jobrunscan/helpers.go
- pkg/api/jobrunscan/symptoms.go
- pkg/sippyclient/client.go
- pkg/api/jobartifacts/query_test.go
- pkg/api/jobartifacts/apitypes.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/api/jobrunscan/labels.go (2)
pkg/api/jobrunscan/helpers.go (1)
ValidIdentifierRegex(13-13)vendor/github.com/invopop/jsonschema/id.go (1)
ID(12-12)
pkg/db/models/job_labels.go (1)
vendor/github.com/invopop/jsonschema/id.go (1)
ID(12-12)
pkg/sippyclient/jobrunscan/symptoms.go (4)
pkg/sippyclient/client.go (1)
Client(21-25)pkg/api/jobrunscan/helpers.go (1)
ValidIdentifierRegex(13-13)pkg/db/models/audit.go (2)
Create(21-21)Update(22-22)vendor/github.com/invopop/jsonschema/id.go (1)
ID(12-12)
🔇 Additional comments (13)
pkg/api/jobrunscan/labels.go (1)
21-21: LGTM! Symbol rename aligns with exported identifier regex.The change from
validIdentifierRegextoValidIdentifierRegexcorrectly references the exported symbol frompkg/api/jobrunscan/helpers.go, maintaining consistent validation across the codebase.pkg/sippyclient/jobrunscan/symptoms.go (5)
12-22: LGTM! Clean client structure and constructor.The
SymptomsClientstruct and constructor follow standard patterns, properly wrapping the basesippyclient.Clientfor symptoms-specific operations.
24-31: LGTM! List method properly handles bulk retrieval.The implementation correctly uses context, performs a GET request, and wraps errors with appropriate context.
33-45: LGTM! Get method includes proper validation.The method validates the ID using
api.ValidIdentifierRegexbefore making the request, preventing invalid IDs from reaching the server.
47-58: LGTM! Create method validates symptom ID.The validation check on line 49 ensures that
symptom.IDconforms to the expected identifier pattern before attempting creation.
77-88: LGTM! Delete method includes proper validation.The method validates the ID before issuing the delete request and provides clear error context.
pkg/api/jobartifacts/query.go (2)
173-195: LGTM! Context propagation and resource management are well-implemented.The addition of the
context.Contextparameter enables proper cancellation and timeout handling. The reader abstraction with thedefer closer()pattern is safe and idiomatic—the closer function handles nil readers gracefully, so deferring it immediately afterOpenArtifactReaderreturns is correct even when there's an error.The preservation of partial matches even when scanning encounters an error (line 193) is intentional and valuable for observability.
197-234: Well-designed reader abstraction with proper resource cleanup.The
OpenArtifactReaderfunction implements a clean pattern for transparently handling gzip decompression. The closer function properly handles nil readers, and the explicit nil assignments before returning errors (lines 217, 225) are defensive and correct.The check for
contentType == "application/gzip"(line 221) correctly identifies gzip-compressed artifacts in the codebase. No other compression formats or gzip content-type variations are currently used.pkg/api/jobartifacts/manager.go (1)
269-269: LGTM! Context propagation correctly threaded through.The worker context is now properly passed to
getFileContentMatches, enabling cancellation and timeout signals to propagate into artifact content matching operations.pkg/db/models/job_labels.go (1)
1-32: LGTM! Clean model definition with helpful deployment documentation.The
JobRunLabelstruct is well-designed with appropriate BigQuery field mappings. The exclusion of theURLfield from BigQuery persistence (line 18) is correct since it's a derived field. The schema migration documentation (lines 21-32) provides clear guidance for both CLI and SQL-based deployments.pkg/componentreadiness/jobrunannotator/jobrunannotator.go (3)
387-396: LGTM! Clean migration to the new model type.The updated signature correctly uses
models.JobRunLabel, and the dry-run logging properly accesses theURLfield from the new model structure.
438-491: LGTM! Consistent model migration in BigQuery fetch logic.The function correctly returns and processes
models.JobRunLabelinstances. The type consistency across the result map, row variable, and return type is correct.
493-523: LGTM! Proper initialization of the new model fields.The creation of
JobRunLabelinstances correctly initializes all new fields:
CreatedAtandUpdatedAtboth set to the current timestamp (line 500, 513-514) is appropriate for new recordsSourceToolhardcoded to identify manual annotations (line 515) is reasonableSymptomIDintentionally left empty per the inline comment (line 516)The migration from the previous annotation type to
models.JobRunLabelis complete and consistent.
|
Scheduling required tests: |
|
/test e2e |
|
@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.