-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(server): Optional timestamp inclusion when retrieving workflow logs. Closes #4033 #4075
Conversation
I have not found any existing tests that test the contents of retrieved logs and I'm not really sure how to test this myself. It would be great if someone could comment on what to do about this. |
logCtx.WithField("options", logOptions).Debug("Log options") | ||
|
||
// make a copy of requested log options and set timestamps to true, so they can be parsed out later | ||
podLogStreamOptions := *logOptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could just use logOptions
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats exactly what the problem was, the logOptions
is a pointer. So when you set timestamps to true in the next line, you lose the original value which was set by user. This is important because line 115 uses the original logOptions
set by user. My fix makes a copy of users logOptions
and sets timestamps to true on the copy only, without altering the original value.
Ok. To test this you need to run
In another terminal:
|
Thanks. All tests pass. I did test earlier with Postman, but now I did the test using Argo CLI. Here's the output: Make test outputivancili@macpro ~/g/s/g/i/argo (fix/optionally-prepend-timestamps-to-pod-logs)> GOPATH=/Users/ivancili/go/ make test
env KUBECONFIG=/dev/null go test ./...
? github.com/argoproj/argo [no test files]
ok github.com/argoproj/argo/api/openapi-spec (cached)
? github.com/argoproj/argo/cmd/argo [no test files]
ok github.com/argoproj/argo/cmd/argo/commands (cached)
? github.com/argoproj/argo/cmd/argo/commands/archive [no test files]
? github.com/argoproj/argo/cmd/argo/commands/auth [no test files]
ok github.com/argoproj/argo/cmd/argo/commands/client (cached)
ok github.com/argoproj/argo/cmd/argo/commands/clustertemplate (cached)
ok github.com/argoproj/argo/cmd/argo/commands/cron (cached)
? github.com/argoproj/argo/cmd/argo/commands/template [no test files]
? github.com/argoproj/argo/cmd/argoexec [no test files]
? github.com/argoproj/argo/cmd/argoexec/commands [no test files]
? github.com/argoproj/argo/cmd/workflow-controller [no test files]
ok github.com/argoproj/argo/config (cached)
ok github.com/argoproj/argo/errors (cached)
? github.com/argoproj/argo/examples/example-golang [no test files]
? github.com/argoproj/argo/hack [no test files]
ok github.com/argoproj/argo/persist/sqldb (cached)
? github.com/argoproj/argo/persist/sqldb/mocks [no test files]
? github.com/argoproj/argo/pkg/apiclient [no test files]
? github.com/argoproj/argo/pkg/apiclient/clusterworkflowtemplate [no test files]
? github.com/argoproj/argo/pkg/apiclient/cronworkflow [no test files]
? github.com/argoproj/argo/pkg/apiclient/event [no test files]
? github.com/argoproj/argo/pkg/apiclient/info [no test files]
? github.com/argoproj/argo/pkg/apiclient/workflow [no test files]
? github.com/argoproj/argo/pkg/apiclient/workflow/mocks [no test files]
? github.com/argoproj/argo/pkg/apiclient/workflowarchive [no test files]
? github.com/argoproj/argo/pkg/apiclient/workflowtemplate [no test files]
? github.com/argoproj/argo/pkg/apis/workflow [no test files]
ok github.com/argoproj/argo/pkg/apis/workflow/v1alpha1 (cached)
? github.com/argoproj/argo/pkg/client/clientset/versioned [no test files]
? github.com/argoproj/argo/pkg/client/clientset/versioned/fake [no test files]
? github.com/argoproj/argo/pkg/client/clientset/versioned/scheme [no test files]
? github.com/argoproj/argo/pkg/client/clientset/versioned/typed/workflow/v1alpha1 [no test files]
? github.com/argoproj/argo/pkg/client/clientset/versioned/typed/workflow/v1alpha1/fake [no test files]
? github.com/argoproj/argo/pkg/client/informers/externalversions [no test files]
? github.com/argoproj/argo/pkg/client/informers/externalversions/internalinterfaces [no test files]
? github.com/argoproj/argo/pkg/client/informers/externalversions/workflow [no test files]
? github.com/argoproj/argo/pkg/client/informers/externalversions/workflow/v1alpha1 [no test files]
? github.com/argoproj/argo/pkg/client/listers/workflow/v1alpha1 [no test files]
? github.com/argoproj/argo/server/apiserver [no test files]
ok github.com/argoproj/argo/server/artifacts (cached)
ok github.com/argoproj/argo/server/auth (cached)
? github.com/argoproj/argo/server/auth/jws [no test files]
ok github.com/argoproj/argo/server/auth/jwt (cached)
? github.com/argoproj/argo/server/auth/mocks [no test files]
ok github.com/argoproj/argo/server/auth/sso (cached)
? github.com/argoproj/argo/server/auth/sso/mocks [no test files]
ok github.com/argoproj/argo/server/auth/webhook (cached)
ok github.com/argoproj/argo/server/clusterworkflowtemplate (cached)
ok github.com/argoproj/argo/server/cronworkflow (cached)
ok github.com/argoproj/argo/server/event (cached)
ok github.com/argoproj/argo/server/event/dispatch (cached)
ok github.com/argoproj/argo/server/info (cached)
? github.com/argoproj/argo/server/static [no test files]
ok github.com/argoproj/argo/server/workflow (cached)
ok github.com/argoproj/argo/server/workflowarchive (cached)
ok github.com/argoproj/argo/server/workflowtemplate (cached)
? github.com/argoproj/argo/test [no test files]
? github.com/argoproj/argo/test/e2e/fixtures [no test files]
ok github.com/argoproj/argo/test/e2e/images/argosay/v2/main (cached)
? github.com/argoproj/argo/test/util [no test files]
ok github.com/argoproj/argo/util (cached)
ok github.com/argoproj/argo/util/archive (cached)
ok github.com/argoproj/argo/util/auth (cached)
ok github.com/argoproj/argo/util/cmd (cached)
ok github.com/argoproj/argo/util/errors (cached)
ok github.com/argoproj/argo/util/file (cached)
? github.com/argoproj/argo/util/grpc [no test files]
? github.com/argoproj/argo/util/help [no test files]
ok github.com/argoproj/argo/util/instanceid (cached)
ok github.com/argoproj/argo/util/intstr (cached)
? github.com/argoproj/argo/util/json [no test files]
ok github.com/argoproj/argo/util/kubeconfig (cached)
ok github.com/argoproj/argo/util/labels (cached)
? github.com/argoproj/argo/util/logs [no test files]
ok github.com/argoproj/argo/util/printer (cached)
ok github.com/argoproj/argo/util/resource (cached)
? github.com/argoproj/argo/util/retry [no test files]
ok github.com/argoproj/argo/util/slice (cached)
? github.com/argoproj/argo/util/unstructured [no test files]
? github.com/argoproj/argo/workflow/artifacts [no test files]
ok github.com/argoproj/argo/workflow/artifacts/artifactory (cached)
? github.com/argoproj/argo/workflow/artifacts/gcs [no test files]
ok github.com/argoproj/argo/workflow/artifacts/git (cached)
? github.com/argoproj/argo/workflow/artifacts/hdfs [no test files]
ok github.com/argoproj/argo/workflow/artifacts/http (cached)
? github.com/argoproj/argo/workflow/artifacts/oss [no test files]
ok github.com/argoproj/argo/workflow/artifacts/raw (cached)
? github.com/argoproj/argo/workflow/artifacts/resource [no test files]
? github.com/argoproj/argo/workflow/artifacts/s3 [no test files]
ok github.com/argoproj/argo/workflow/common (cached)
ok github.com/argoproj/argo/workflow/controller (cached)
? github.com/argoproj/argo/workflow/controller/cache [no test files]
ok github.com/argoproj/argo/workflow/controller/informer (cached)
ok github.com/argoproj/argo/workflow/controller/pod (cached)
ok github.com/argoproj/argo/workflow/creator (cached)
ok github.com/argoproj/argo/workflow/cron (cached)
? github.com/argoproj/argo/workflow/events [no test files]
ok github.com/argoproj/argo/workflow/executor (cached)
? github.com/argoproj/argo/workflow/executor/common [no test files]
? github.com/argoproj/argo/workflow/executor/docker [no test files]
? github.com/argoproj/argo/workflow/executor/k8sapi [no test files]
? github.com/argoproj/argo/workflow/executor/kubelet [no test files]
? github.com/argoproj/argo/workflow/executor/mocks [no test files]
? github.com/argoproj/argo/workflow/executor/os-specific [no test files]
? github.com/argoproj/argo/workflow/executor/pns [no test files]
ok github.com/argoproj/argo/workflow/hydrator (cached)
ok github.com/argoproj/argo/workflow/hydrator/fake (cached)
ok github.com/argoproj/argo/workflow/metrics (cached)
ok github.com/argoproj/argo/workflow/packer (cached)
ok github.com/argoproj/argo/workflow/sync (cached)
ok github.com/argoproj/argo/workflow/templateresolution (cached)
ok github.com/argoproj/argo/workflow/ttlcontroller (cached)
ok github.com/argoproj/argo/workflow/util (cached)
ok github.com/argoproj/argo/workflow/validate (cached)
ivancili@macpro ~/g/s/g/i/argo (fix/optionally-prepend-timestamps-to-pod-logs)> Argo CLI test outputivancili@macpro ~/g/s/g/i/argo (fix/optionally-prepend-timestamps-to-pod-logs)> ./dist/argo logs hello-world-hkpw2 -c main -s localhost:2746 -n argo
hello-world-hkpw2: _____________
hello-world-hkpw2: < hello world >
hello-world-hkpw2: -------------
hello-world-hkpw2: \
hello-world-hkpw2: \
hello-world-hkpw2: \
hello-world-hkpw2: ## .
hello-world-hkpw2: ## ## ## ==
hello-world-hkpw2: ## ## ## ## ===
hello-world-hkpw2: /""""""""""""""""___/ ===
hello-world-hkpw2: ~~~ {~~ ~~~~ ~~~ ~~~~ ~~ ~ / ===- ~~~
hello-world-hkpw2: \______ o __/
hello-world-hkpw2: \ \ __/
hello-world-hkpw2: \____\______/
ivancili@macpro ~/g/s/g/i/argo (fix/optionally-prepend-timestamps-to-pod-logs)> ./dist/argo logs hello-world-hkpw2 -c main -s localhost:2746 -n argo --timestamps
hello-world-hkpw2: 2020-09-21T21:00:37.187393980Z _____________
hello-world-hkpw2: 2020-09-21T21:00:37.187431954Z < hello world >
hello-world-hkpw2: 2020-09-21T21:00:37.187436600Z -------------
hello-world-hkpw2: 2020-09-21T21:00:37.187440013Z \
hello-world-hkpw2: 2020-09-21T21:00:37.187444733Z \
hello-world-hkpw2: 2020-09-21T21:00:37.187447981Z \
hello-world-hkpw2: 2020-09-21T21:00:37.187451267Z ## .
hello-world-hkpw2: 2020-09-21T21:00:37.187455118Z ## ## ## ==
hello-world-hkpw2: 2020-09-21T21:00:37.187458287Z ## ## ## ## ===
hello-world-hkpw2: 2020-09-21T21:00:37.187461776Z /""""""""""""""""___/ ===
hello-world-hkpw2: 2020-09-21T21:00:37.187465790Z ~~~ {~~ ~~~~ ~~~ ~~~~ ~~ ~ / ===- ~~~
hello-world-hkpw2: 2020-09-21T21:00:37.187468977Z \______ o __/
hello-world-hkpw2: 2020-09-21T21:00:37.187472105Z \ \ __/
hello-world-hkpw2: 2020-09-21T21:00:37.187475445Z \____\______/
ivancili@macpro ~/g/s/g/i/argo (fix/optionally-prepend-timestamps-to-pod-logs)> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
…gs. Closes argoproj#4033 (argoproj#4075) Signed-off-by: Alex Capras <alexcapras@gmail.com>
Checklist:
"fix(controller): Updates such and such. Fixes #1234"
.Closes #4033