Add extension points to cilium-envoy ds#42223
Conversation
|
/test |
|
Commit 100e485 does not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
100e485 to
4801370
Compare
|
/test |
|
Commit 3e66042 does not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
3e66042 to
2d58c7f
Compare
|
Commit 3e66042 does not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
|
/test |
Artyop
left a comment
There was a problem hiding this comment.
two empty commits that needs to be removed please !
if you want to use github GUI when the branch isn't up to date I think you can chose between "rebase" or "merge", picking the rebase option will not create a new commit AFAIK
2d58c7f to
f57813e
Compare
|
/test |
|
@Artyop am not yet familiar with cilium PR contribution guidelines, but one could also squash all commits into one upon merge to main? E.g. rebasing overrides commit history and review comments may get lost. |
Well for this it's a bit up to you, I like to have a feature per commit but as long as your commit messages are clear on what is the purpose of the commit and how it works, you're good |
ea6ceee to
b0cab43
Compare
|
/test |
install/kubernetes/cilium/templates/cilium-envoy/daemonset.yaml
Outdated
Show resolved
Hide resolved
In general this makes sense, but in practice the common workflow for merging code in Cilium is that a @cilium/committers clicks the green button at the bottom of the page to "merge when ready". With this pattern, a committer makes the determination if they're happy merging the PR and queues the merge for the future. Then GitHub will wait for all code owners to approve, comment threads to be resolved, and tests to pass, then it will merge automatically. With this workflow it's less natural for the committer to pull the PR, amend it, push to your repository and then merge. Most Cilium contributors would follow the pattern of force-pushing the branch when addressing feedback, and in the end we aim to have a bisectable tree with each commit introducing a separate logical change (notably, refactors should be separate from functional changes). I believe that the feedback threads are hidden by GitHub UI but still accessible from "breadcrumbs" on the PR page. |
|
I'll hold back on merging this for now despite 'ready-to-merge' label just because I'm not sure if case sensitivity mentioned in my review is important. |
b0cab43 to
ec260b6
Compare
Signed-off-by: Kateryna Nezdolii <kateryna.nezdolii@isovalent.com>
ec260b6 to
81eaed5
Compare
|
@joestringer fixed the case sensitivity, it was not important for feature to work, but did break the naming style conventions used in the rest of the file |
|
/test |
| # -- Init containers added to the cilium Envoy DaemonSet. | ||
| initContainers: [] |
There was a problem hiding this comment.
Sorry for the late drive-by review.
But AFAICT this is not actually implemented. This PR only adds extension points and doesn't actually reference this helm value. Or am I missing something?
cilium/cilium#44749 cilium/cilium#43887 cilium/cilium#42359 cilium/cilium#42223 cilium/cilium#42232 Signed-off-by: Kateryna Nezdolii <kateryna.nezdolii@gmail.com>
cilium/cilium#44749 cilium/cilium#43887 cilium/cilium#42359 cilium/cilium#42223 cilium/cilium#42232 Signed-off-by: Kateryna Nezdolii <kateryna.nezdolii@gmail.com>
cilium/cilium#44749 cilium/cilium#43887 cilium/cilium#42359 cilium/cilium#42223 cilium/cilium#42232 Signed-off-by: Kateryna Nezdolii <kateryna.nezdolii@gmail.com>
cilium/cilium#44749 cilium/cilium#43887 cilium/cilium#42359 cilium/cilium#42223 cilium/cilium#42232 Signed-off-by: Kateryna Nezdolii <kateryna.nezdolii@gmail.com>
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXXline if the commit addresses a particularGitHub issue.
Fixes: <commit-id>tag, thenplease add the commit author[s] as reviewer[s] to this issue.
This change adds multiple extension points for cilium envoy daemonset manifest.
Fixes: #issue-number