-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 log-format value to the multicluster-link helm chart #10779
Conversation
Hi @bunnybilou, you'll need to run |
7d084b0
to
e83127b
Compare
@adleong did the required changes to add |
e83127b
to
480611e
Compare
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.
It looks like this has broken the link
command: https://github.com/linkerd/linkerd2/actions/runs/4828092048/jobs/8601723773?pr=10779
Failing test has this dependency. I have modified the multicluster valus.go file but we need to publish it to make it work. How do I update the library? |
The tests use the code and charts directly from the repo. No publishing is necessary. |
3f23eda
to
05a8e89
Compare
multicluster/cmd/link.go
Outdated
@@ -398,6 +401,10 @@ func buildServiceMirrorValues(opts *linkOptions) (*multicluster.Values, error) { | |||
return nil, fmt.Errorf("--log-level must be one of: panic, fatal, error, warn, info, debug") | |||
} | |||
|
|||
if _, err := log.ParseLevel(opts.logFormat); err != nil { |
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.
This call will always fail
Hi @bunnybilou, are you still interested on bringing this one across the finish line? :-) |
05a8e89
to
3adb8e6
Compare
Hi @alpeb thank you for your review. I have fixed it, let me know if this is ok for 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.
Looks good to me!
multicluster/cmd/link.go
Outdated
if opts.logFormat == "" { | ||
return nil, fmt.Errorf("--log-format must be one of: plain, json") | ||
} | ||
|
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.
Can you have here a more specific check, that validates that the value is either plain
or json
?
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.
@alpeb, I made the change but one test is failing Integration tests / test-multicluster (v1.26) (pull_request) but I don't think it's related to the modification.
09c206d
to
5169f4c
Compare
Signed-off-by: Arnaud Beun <arnaud.beun@sorare.com>
5169f4c
to
79b8c8e
Compare
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.
Thanks again @bunnybilou , the tests failures were just flakiness. This is ready to go 👍
Make log-format configurable on the multicluster-link helm chart
Signed-off-by: Arnaud Beun arnaud.beun@sorare.com