-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
[weave] Add support for default version override #10273
[weave] Add support for default version override #10273
Conversation
Hi @dntosas. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
6e556f2
to
b7f7385
Compare
pkg/apis/kops/networking.go
Outdated
|
||
// Version overrides the default tag of Weave container image. | ||
Version string `json:"version,omitempty"` |
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.
@rifelpet any thoughts if to use Version or ImageName, seems very inconsistent?
// Version overrides the default tag of Weave container image. | |
Version string `json:"version,omitempty"` | |
// Version overrides the default tag of Weave container image. | |
Version string `json:"version,omitempty"` |
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.
just for reference, i followed this logic -->
kops/upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.12.yaml.template
Line 446 in 27f9114
image: "docker.io/cilium/cilium:{{ .Version }}" |
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.
No worries, just asking @rifelpet if there's a preferred way to do this.
b7f7385
to
e9a172e
Compare
I think you are missing a generated file. Can you check if you missed anything from |
pkg/apis/kops/networking.go
Outdated
@@ -86,6 +86,9 @@ type WeaveNetworkingSpec struct { | |||
NPCCPULimit *resource.Quantity `json:"npcCPULimit,omitempty"` | |||
// NPCExtraArgs are extra arguments that are passed to weave-npc. | |||
NPCExtraArgs string `json:"npcExtraArgs,omitempty"` | |||
|
|||
// Version overrides the default tag of Weave container image. |
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 would suggest de-emphasizing the default. Something like "Version specifies the Weave container image tag. The default depends on the kOps version."
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 for your suggestion @johngmyers , pushed changes accordingly ^^
e9a172e
to
a3b759a
Compare
i have the same problem again, running |
7d579c1
to
a6bc403
Compare
Looks pretty much ok now. |
a6bc403
to
64e246f
Compare
Sorry, was ok before with the extra line. Didn't see that all groups are separated in that struct. |
yes 😃 ^^ Remember i had the same problem last time and we did the same conversation again, pointing me to put git repo into
Should i add extra line or not? |
Please add it back. |
4256d74
to
9e4832e
Compare
/ok-to-test |
In this commit, we enable users to override default version of networking/Weave specs. Signed-off-by: dntosas <ntosas@gmail.com>
9e4832e
to
b7a2d0a
Compare
added some docs as reference ^^ |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dntosas, hakman The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…-upstream-release-1.19 Automated cherry pick of #10273: Add support for default version override
…-upstream-release-1.18 Automated cherry pick of #10273: Add support for default version override
In this PR, we enable users to override the default version of networking/Weave specs.