fix(tektonresult): default watcher logs_api from spec - #3884
Open
pujitha24 wants to merge 1 commit into
Open
Conversation
Motivation: setting spec.logs_api: true on a TektonResult (or via
TektonConfig) enables the LOGS_API env var on the tekton-results-api
deployment, but the tekton-results-watcher deployment has its own,
separate -logs_api command-line flag that defaults to false. That
flag is what actually makes the Watcher forward TaskRun/PipelineRun
logs to the API server; nothing defaulted it from the top-level
logs_api setting, so users following the documented example (setting
only the top-level logs_api) got an API server with logging enabled
but a Watcher that never sends logs, and logs were never stored.
Approach: default Result.Watcher.LogsAPI from the top-level
ResultsAPIProperties.LogsAPI in Result.setDefaults() whenever the
user has not explicitly set spec.watcher.logs_api, mirroring the
adjacent RouteEnabled/RouteTLSTermination defaulting already in that
function. An explicit spec.watcher.logs_api value is never
overridden.
This defaulting runs wherever Result.setDefaults() runs today, i.e.
when a TektonResult is created/updated via TektonConfig (the
documented install path). TektonResult.SetDefaults does not call
Result.setDefaults() at all, so a CR applied directly as a
standalone `kind: TektonResult` (bypassing TektonConfig) does not
get this or the pre-existing RouteEnabled default either; that gap
predates this change and is not addressed here.
Validation: added TestResult_SetDefaultsWatcherLogsAPI, a table test
covering propagate-true, propagate-false, no-override-when-explicit,
and no-op-when-top-level-unset. Confirmed by temporarily reverting
the fix that the two propagation subtests fail without it and pass
with it:
go test ./pkg/apis/operator/v1alpha1/... \
-run TestResult_SetDefaultsWatcherLogsAPI -v
Also ran, all passing:
go build ./...
go test ./pkg/apis/operator/v1alpha1/... \
./pkg/reconciler/kubernetes/tektonresult/... \
./pkg/reconciler/kubernetes/tektonconfig/...
gofmt -l on both changed files (clean)
golangci-lint could not be run in this sandbox (its download step
fails a checksum check with no network access); go vet ./pkg/apis/
operator/v1alpha1/... was run instead and reported nothing.
Report: tektoncd#2600
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
tekton-robot
added
the
release-note
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
tekton-robot
added
the
size/L
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3884 +/- ##
=======================================
Coverage 26.13% 26.13%
=======================================
Files 465 465
Lines 24933 24935 +2
=======================================
+ Hits 6516 6518 +2
Misses 17697 17697
Partials 720 720
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
/retest |
1 similar comment
Contributor
Author
|
/retest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Fixes a bug where setting
spec.logs_api: trueon aTektonResult(or viaTektonConfig) enabled theLOGS_APIenv var on thetekton-results-apideployment, but did not propagate to thetekton-results-watcherdeployment's separate-logs_apiflag (which defaults tofalse). As a result, users following the documented example of only setting the top-levellogs_apiended up with an API server that had logging enabled but a Watcher that never forwarded logs, so logs were never stored.Result.setDefaults()now defaultsResult.Watcher.LogsAPIfrom the top-levelResultsAPIProperties.LogsAPIwhenever the user has not explicitly setspec.watcher.logs_api, mirroring the existingRouteEnabled/RouteTLSTerminationdefaulting in the same function. An explicitspec.watcher.logs_apivalue is never overridden.Note: this defaulting runs wherever
Result.setDefaults()runs today (i.e. when aTektonResultis created/updated viaTektonConfig, the documented install path).TektonResult.SetDefaultsdoes not callResult.setDefaults(), so a CR applied directly as a standalonekind: TektonResultdoes not get this (or the pre-existingRouteEnabled) default — that gap predates this change and is not addressed here.Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
make test lintbefore submitting a PRSee the contribution guide for more details.
Release Notes
AI assistance: this change was drafted with Claude Code.
Fixes #2600