-
Notifications
You must be signed in to change notification settings - Fork 38.8k
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
Fix bug in apiserver service cidr split #85937
Conversation
Hi @darshanime. 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. |
/ok-to-test |
/sig network |
/lgtm |
be19ecd
to
dd6cd25
Compare
thanks |
@darshanime -- This needs a release note. |
Also needs a test |
New changes are detected. LGTM label has been removed. |
Signed-off-by: darshanime <deathbullet@gmail.com>
b875cc3
to
c0f795d
Compare
@liggitt, I have removed the default |
Don't change the integration test setup. It would be better to have the apiServerServiceIP, serviceIPRange, secondaryServiceIPRange, err := getServiceIPAndRanges(s.ServiceClusterIPRanges)
if err != nil {
return options, err
}
s.PrimaryServiceClusterIPRange = serviceIPRange
s.SecondaryServiceClusterIPRange = secondaryServiceIPRange |
Signed-off-by: darshanime <deathbullet@gmail.com>
71d6c55
to
2c64a45
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: darshanime The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@liggitt thanks for the review. Please take a look now. |
need to run diff --git a/cmd/kube-apiserver/app/BUILD b/cmd/kube-apiserver/app/BUILD
index 66570c567e5..95162cf8760 100644
--- a/cmd/kube-apiserver/app/BUILD
+++ b/cmd/kube-apiserver/app/BUILD
@@ -1,4 +1,4 @@
-load("@io_bazel_rules_go//go:def.bzl", "go_library")
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
go_library(
name = "go_default_library",
@@ -99,3 +99,9 @@ filegroup(
tags = ["automanaged"],
visibility = ["//visibility:public"],
)
+
+go_test(
+ name = "go_default_test",
+ srcs = ["server_test.go"],
+ embed = [":go_default_library"],
+)
lgtm otherwise, much better tested, thanks! |
@@ -0,0 +1,56 @@ | |||
/* | |||
Copyright 2018 The Kubernetes Authors. |
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.
2019 in new files
@darshanime: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
comments resolved in #85968 |
/close |
@liggitt: Closed this PR. In response to this:
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. |
Hi @liggitt, sorry I was asleep, it was already 1am IST here 😴 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fix bug where kube-apiserver would fail to start when not providing service CIDR
Which issue(s) this PR fixes:
NA
Special notes for your reviewer:
The
strings.Split
documentation states:playground link
This would fail the if check here and apiserver wouldn't start. This PR fixes this bug.
Does this PR introduce a user-facing change?:
NA
Release note: