feat(logs): pass preferences to external providers - #5228
akashchamp wants to merge 1 commit into
Conversation
|
[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 |
|
@akashchamp as mentioned on your previous PR, the Tekton community has an AI contribution policy that requires AI usage to be disclosed. As far as I can see, all that has changed in this PR compared to the previous one is that you have renamed the branch to remove reference to 'codex'. Please review the policy and update your PR appropriately to comply with the requirements. There is no need to close the PR and open a new one, you should be able to edit the commit message and PR description instead. |
dc2f21f to
d89d947
Compare
|
@AlanGreene I have updated the description |
AlanGreene
left a comment
There was a problem hiding this comment.
Thanks for the PR @akashchamp, overall it looks good. I've added a few comments with minor suggestions, let me know if you have any questions.
External log providers only received the step start and completion times when the Dashboard fell back to the external logs endpoint, so they had no way to honour the user's timestamp and log level preferences from the log toolbar. Pass the preferences to external log providers as `timestamps` and repeated `logLevel` query parameters on the external log URL, and propagate them through the PipelineRun and TaskRun fallback requests and the raw log links in the step log toolbar. Only `namespace`, `podName` and `container` are required; providers may ignore the other parameters. Also drop the `externalLogsURL` and `isUsingExternalLogs` props that were passed to the run-level LogsToolbar instead of the StepLogToolbar in an earlier refactoring. Fixes tektoncd#4981 Assisted-by: Codex Assisted-by: Claude Code Signed-off-by: Akash Kumar <116457960+akashchamp@users.noreply.github.com>
d89d947 to
ae45ba0
Compare
|
Thanks for the pointer to the policy. I have updated the commit message with Assisted-by trailers for the tools used (Codex for the original implementation and Claude Code for the revision addressing your review) and added a matching AI disclosure section to the PR description that states the extent of the assistance. The review comments have been addressed in the same commit. |
This seems to suggest that this change hasn't actually been validated in a live environment. Is that correct? The logs persistence walk-through provides an example of how an environment can be configured to validate this change end to end. At a minimum, the Dashboard deployment can be updated to provide a dummy value for the external-logs URL. Once the Dashboard has restarted to pick up the config change, verify in the browser that the log fallback process still works as expected when a TaskRun pod has been deleted, and that the resulting URL via the logs proxy contains the expected params. |
Changes
Passes the user's timestamp and selected log-level preferences to external log providers.
timestamps=<true|false>and repeatedlogLevelparameters to external log URLs.namespace,podName, andcontainerare required; providers may ignore the rest.externalLogsURLandisUsingExternalLogsprops that were passed to the run-levelLogsToolbarinstead of theStepLogToolbarin an earlier refactoring.Fixes #4981
Validation: targeted tests (137), the full test suite (614 passing, 1 skipped), lint, and production build pass. A live UI check requires an authenticated Tekton cluster and configured external log provider; neither is available on the remote host, so no cluster state or credentials were changed.
Submitter Checklist
Release Notes
AI disclosure
This PR was developed with AI assistance, disclosed per the Tekton AI contribution policy:
Assisted-by: Codexcommit trailer; the earlier PR feat(logs): pass preferences to external providers. Fixes #4981 #5227 was opened from acodex/...branch.showTimestampsrename, the single-object argument forfetchLogsFallback, removing the misplacedLogsToolbarprops, the header years, and the docs wording) was made with Claude Code (Anthropic), recorded by theAssisted-by: Claude Codecommit trailer.For the revised change,
npm run lintand the fullvitestsuite were run locally and pass.