-
Notifications
You must be signed in to change notification settings - Fork 156
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
Add k8s e2e test #5399
Add k8s e2e test #5399
Conversation
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
7c2f5c6
to
9ddcc8e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5399 +/- ##
==========================================
+ Coverage 25.84% 26.10% +0.25%
==========================================
Files 448 451 +3
Lines 48255 48824 +569
==========================================
+ Hits 12473 12747 +274
- Misses 34810 35059 +249
- Partials 972 1018 +46 ☔ View full report in Codecov by Sentry. |
@@ -92,7 +92,7 @@ func newConfig(level zapcore.Level, encoding EncodingType) zap.Config { | |||
func newEncoderConfig(encoding EncodingType) zapcore.EncoderConfig { | |||
if encoding == HumanizeEncoding { | |||
return zapcore.EncoderConfig{ | |||
TimeKey: "eventTime", | |||
TimeKey: "", |
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.
After zap 1.14.1, we need to set EncodeTime
if we set TimeKey
. So the test for piped's logger is failed.
uber-go/zap#791
But currently, piped's logger doesn't show a timestamp when the log encoding is HumanizeEncoding
.
So I fixed not to set TimeKey
.
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.
[question] How does this affect other log encoding settings (which are not humanized)? Afaik, some teams are using piped logs with JSON encoding now, so we need to ensure nothing is broken.
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.
@khanhtc1202
I think it doesn't affect other log formats because EncodeTime
is set for the other format.
https://github.com/pipe-cd/pipecd/blob/9ddcc8e9f80d441a05b6873a3719df5c79cca002/pkg/log/log.go#L92C1-L121C2
The problem is that when not set EncodeTime
despite TimeKey
is set.
uber-go/zap#791
I also checked the log for each formats below and eventTime
is inserted when using json and console log format.
json format
% CONFIG_FILE=./.dev/local2/piped.yaml LOG_ENCODING=json make run/piped (git)-[add-k8s-e2e-test]
go run cmd/piped/main.go piped --tools-dir=/tmp/piped-bin --config-file=./.dev/local2/piped.yaml --insecure=false --log-encoding=json
{"severity":"INFO","eventTime":1734398586.566765,"logger":"piped.piped","caller":"piped/piped.go:182","message":"successfully configured ssh-config","serviceContext":{"service":"piped.piped","version":"unspecified"}}
{"severity":"INFO","eventTime":1734398586.568584,"logger":"piped.piped.tool-registry","caller":"toolregistry/registry.go:57","message":"successfully loaded the pre-installed tools","serviceContext":{"service":"piped.piped","version":"unspecified"},"tools":{"helm":{},"kubectl-1.18.5":{},"kubectl-1.30.0":{},"kubectl-1.30.5":{},"kustomize":{}}}
{"severity":"INFO","eventTime":1734398589.319672,"logger":"piped.piped.notifier","caller":"notifier/notifier.go:116","message":"all 0 notifiers have been started","serviceContext":{"service":"piped.piped","version":"unspecified"}}
...
console format
% CONFIG_FILE=./.dev/local2/piped.yaml LOG_ENCODING=console make run/piped (git)-[add-k8s-e2e-test]
go run cmd/piped/main.go piped --tools-dir=/tmp/piped-bin --config-file=./.dev/local2/piped.yaml --insecure=false --log-encoding=console
1.734398656024647e+09 INFO piped.piped piped/piped.go:182 successfully configured ssh-config {"serviceContext": {"service": "piped.piped", "version": "unspecified"}}
1.734398656024822e+09 INFO piped.piped.tool-registry toolregistry/registry.go:57 successfully loaded the pre-installed tools {"serviceContext": {"service": "piped.piped", "version": "unspecified"}, "tools": {"helm":{},"kubectl-1.18.5":{},"kubectl-1.30.0":{},"kubectl-1.30.5":{},"kustomize":{}}}
1.734398657369169e+09 INFO piped.piped.notifier notifier/notifier.go:116 all 0 notifiers have been started {"serviceContext": {"service": "piped.piped", "version": "unspecified"}}
...
humanize format
% CONFIG_FILE=./.dev/local2/piped.yaml LOG_ENCODING=humanize make run/piped (git)-[add-k8s-e2e-test]
go run cmd/piped/main.go piped --tools-dir=/tmp/piped-bin --config-file=./.dev/local2/piped.yaml --insecure=false --log-encoding=humanize
successfully configured ssh-config
successfully loaded the pre-installed tools {"tools": {"helm":{},"kubectl-1.18.5":{},"kubectl-1.30.0":{},"kubectl-1.30.5":{},"kustomize":{},"kustomize-5.0.1":{},"kustomize-5.3.0":{}}}
all 0 notifiers have been started
I checked, and there are no notable changes for the release of this updated dependency below.
Also, I fixed the cause of failing test for zap. |
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 👍🏻
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.
Thank you 👍
Makefile
Outdated
@@ -76,13 +76,16 @@ test: test/go test/web | |||
test/go: COVERAGE ?= false | |||
test/go: COVERAGE_OPTS ?= -covermode=atomic | |||
test/go: COVERAGE_OUTPUT ?= coverage.out | |||
test/go: setup-envtest | |||
test/go: ENVTEST_BIN ?= ${PWD}/.dev/bin |
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.
test/go: ENVTEST_BIN ?= ${PWD}/.dev/bin | |
test/go: ENVTEST_BIN ?= ./.dev/bin |
This should be fine
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.
@khanhtc1202
We need it as absolute path for setup-envtest. So I want to keep it as is.
I added the comment.
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.
Got it and reapproved 👍
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
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.
👍 Thank you
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.
👍🏻
What this PR does:
This is the draft test for k8s ensureSync.
Just for reference implementation.
Why we need it:
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: