Skip to content

Fix a typo in the comment example #21402

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

Merged
merged 1 commit into from
Sep 26, 2022
Merged

Fix a typo in the comment example #21402

merged 1 commit into from
Sep 26, 2022

Conversation

farcaller
Copy link
Contributor

It's log in the config.go

Signed-off-by: Vladimir Pouzanov [email protected]

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

Fixes a typo in the comment example: it's log in the config.go

@farcaller farcaller requested review from a team as code owners September 22, 2022 12:13
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 22, 2022
@farcaller
Copy link
Contributor Author

Actually looking into it more, it should be separated with spaces for it to work. cobra is somewhat inconsistent in that regard, so I verified that on practice. Should I patch that in this same PR?

Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @farcaller and thank you for the PR!

Actually looking into it more, it should be separated with spaces for it to work. cobra is somewhat inconsistent in that regard, so I verified that on practice. Should I patch that in this same PR?

That make sense to me, please see also the required helm message here, I think it needs s/logs/log/ and the example should be copied over to the values (possibly after fixing comma vs space).

To fix this CI failure, please edit your changes to install/kubernetes/cilium/values.yaml.tmpl and then run make -C install/kubernetes cilium/values.yaml to update install/kubernetes/cilium/values.yaml.

Finally, pulling someone from the @cilium/api team to confirm the fixes.

@kaworu kaworu added kind/bug This is a bug in the Cilium logic. area/helm Impacts helm charts and user deployment experience release-note/misc This PR makes changes that have no direct user impact. needs-backport/1.10 labels Sep 22, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 22, 2022
@kaworu kaworu requested review from a team and aanm and removed request for a team September 22, 2022 14:23
It's `log` in the [config.go](https://github.com/cilium/cilium/blob/master/pkg/option/config.go#L988).

Also fix the delimeter (the actual cli acceps the space but not a comma).

Signed-off-by: Vladimir Pouzanov <[email protected]>
@farcaller
Copy link
Contributor Author

Done and done.

@aanm aanm requested a review from kaworu September 22, 2022 20:25
@aanm
Copy link
Member

aanm commented Sep 22, 2022

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 23, 2022
@gandro gandro merged commit 3e0d679 into cilium:master Sep 26, 2022
@ti-mo ti-mo added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. backport-done/1.10 and removed backport-pending/1.12 labels Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Impacts helm charts and user deployment experience backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants