Add options to GetContainerLogs #12527
Conversation
Signed-off-by: sheidkamp <[email protected]>
github-actions
bot
added
kind/cleanup
There was a problem hiding this comment.
Pull Request Overview
Adds a variadic options mechanism to GetContainerLogs to allow passing kubectl log flags (container, since, since-time, tail) and introduces helper utilities and tests for building those arguments.
- Introduces LogOption pattern and BuildLogArgs/FormatSinceTime helpers.
- Modifies GetContainerLogs signature to accept optional LogOption values.
- Adds unit tests for argument construction (individual and combined cases).
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/utils/kubeutils/kubectl/logs_options.go | Adds LogOption implementations and argument builder for kubectl logs flags. |
| pkg/utils/kubeutils/kubectl/logs_options_test.go | Adds tests for BuildLogArgs and time formatting helpers. |
| pkg/utils/kubeutils/kubectl/cli.go | Extends GetContainerLogs to accept variadic log options and forwards them to Execute. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: sheidkamp <[email protected]>
sheidkamp
changed the title
pkg/utils/kubeutils/kubectl/cli.go
Outdated
| } | ||
|
|
||
| // GetContainerLogsWithOptions retrieves the logs for the specified container with the specified options | ||
| func (c *Cli) GetContainerLogsWithOptions(ctx context.Context, namespace string, name string, options ...LogOption) (string, error) { |
There was a problem hiding this comment.
since options is optional, looks like you could just add the new arg to the existing GetContainerLogs function instead of defining a new func
There was a problem hiding this comment.
technically a breaking change (if the type is being used anywhere), but reverted to avoid cruft
There was a problem hiding this comment.
Talked offline, but just to document it here: ok, I saw copilot's comment above that it could be a breaking change. Since this is only used in tests, and it doesn't break any of the current callers, I would suggest ignoring copilot and just adding the arg to the existing func.
Also (doesn't need to be done in this PR) it would be good to move the func to a pkg under test/ since it's only used in tests
Signed-off-by: sheidkamp <[email protected]>
sheidkamp
force-pushed
the
sah/container-log-opts
branch
from
0e5923e to
f9f5312
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Signed-off-by: sheidkamp <[email protected]>
Description
I am using the kubeutils as a library in another repo, and would like to be able to pass options (specifically
since-time) to GetContainerLogs.While implementing the changes, added a few more options that may be useful in a testing context:
Change Type
Select one or more of the following by including the corresponding slash-command:
Changelog
Additional Notes