craigcondit commented on code in PR #554:
URL: https://github.com/apache/yunikorn-k8shim/pull/554#discussion_r1146316318
##########
test/e2e/framework/helpers/k8s/k8s_utils.go:
##########
@@ -1114,3 +1114,79 @@ func (k *KubeCtl) DescribeNode(node v1.Node) error {
ginkgo.By("describe output for node is:\n" + out)
return nil
}
+
+func (k *KubeCtl) SetNodeLabel(name, key, value string) error {
Review Comment:
This section looks like it is completely unrelated to the current PR. Can we
remove theses changes please?
##########
go.sum:
##########
@@ -660,15 +667,35 @@ github.com/onsi/ginkgo v1.6.0/go.mod
h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+W
github.com/onsi/ginkgo v1.8.0/go.mod
h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE=
github.com/onsi/ginkgo v1.10.1/go.mod
h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE=
github.com/onsi/ginkgo v1.12.1/go.mod
h1:zj2OWP4+oCPe1qIXoGWkgMRwljMUYCdkwsT2108oapk=
-github.com/onsi/ginkgo v1.14.0 h1:2mOpI4JVVPBN+WQRa0WKH2eXR+Ey+uK4n7Zj0aYpIQA=
github.com/onsi/ginkgo v1.14.0/go.mod
h1:iSB4RoI2tjJc9BBv4NKIKWKya62Rps+oPG/Lv9klQyY=
+github.com/onsi/ginkgo v1.16.4 h1:29JGrr5oVBm5ulCWet69zQkzWipVXIol6ygQUe/EzNc=
Review Comment:
Can we do a `go mod tidy` to clean these up? I can't believe we need so many
versions present...
##########
test/e2e/framework/helpers/k8s/events.go:
##########
@@ -25,16 +25,17 @@ import (
"sync"
"time"
+ "github.com/onsi/gomega"
+
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/apimachinery/pkg/watch"
clientset "k8s.io/client-go/kubernetes"
"k8s.io/client-go/tools/cache"
- "k8s.io/kubernetes/test/e2e/framework"
- "github.com/onsi/ginkgo"
+ "github.com/onsi/ginkgo/v2"
Review Comment:
Minor nit: This should go up around line 29 next to the other github imports.
##########
test/e2e/predicates/predicates_test.go:
##########
@@ -91,10 +85,13 @@ var _ = Describe("Predicates", func() {
Ω(kClient.SetClient()).To(BeNil())
// Initializing rest client
restClient = yunikorn.RClient{}
- nodeList = &v1.NodeList{}
- nodeList, err =
e2enode.GetReadySchedulableNodes(kClient.GetClient())
+
Review Comment:
This change looks unrelated to the current PR. Please remove.
##########
test/e2e/predicates/predicates_suite_test.go:
##########
@@ -24,23 +24,20 @@ import (
v1 "k8s.io/api/core/v1"
- "github.com/onsi/ginkgo/extensions/table"
- "github.com/onsi/ginkgo/reporters"
+ "github.com/onsi/ginkgo/v2/reporters"
"github.com/apache/yunikorn-k8shim/test/e2e/framework/configmanager"
"github.com/apache/yunikorn-k8shim/test/e2e/framework/helpers/common"
- "github.com/apache/yunikorn-k8shim/test/e2e/framework/helpers/k8s"
"github.com/apache/yunikorn-k8shim/test/e2e/framework/helpers/yunikorn"
- "github.com/onsi/ginkgo"
+ "github.com/onsi/ginkgo/v2"
Review Comment:
Can we move the github imports up to the top to be consistent with ordering
with other files?
##########
test/e2e/predicates/predicates_test.go:
##########
@@ -203,8 +203,8 @@ var _ = Describe("Predicates", func() {
initPod, podErr := k8s.InitTestPod(conf)
Ω(podErr).NotTo(HaveOccurred())
- _, err = kClient.CreatePod(initPod, ns)
- Ω(err).NotTo(HaveOccurred())
+ _, existsErr = kClient.CreatePod(initPod, ns)
Review Comment:
This change looks unrelated to the current PR. Please remove.
##########
test/e2e/predicates/predicates_test.go:
##########
@@ -276,9 +276,15 @@ var _ = Describe("Predicates", func() {
By("Trying to apply a random label on the found node")
key := fmt.Sprintf("kubernetes.io/e2e-%s", common.RandSeq(10))
value := "102"
- framework.AddOrUpdateLabelOnNode(kClient.GetClient(), nodeName,
key, value)
- framework.ExpectNodeHasLabel(kClient.GetClient(), nodeName,
key, value)
- defer framework.RemoveLabelOffNode(kClient.GetClient(),
nodeName, key)
+ labelErr := kClient.SetNodeLabel(nodeName, key, value)
Review Comment:
This change looks unrelated to the current PR. Please remove.
##########
test/e2e/predicates/predicates_test.go:
##########
@@ -183,9 +177,15 @@ var _ = Describe("Predicates", func() {
By("Trying to apply a random label on the found node")
key := fmt.Sprintf("kubernetes.io/e2e-%s", common.RandSeq(10))
value := "101"
- framework.AddOrUpdateLabelOnNode(kClient.GetClient(), nodeName,
key, value)
- framework.ExpectNodeHasLabel(kClient.GetClient(), nodeName,
key, value)
- defer framework.RemoveLabelOffNode(kClient.GetClient(),
nodeName, key)
+ labelErr := kClient.SetNodeLabel(nodeName, key, value)
Review Comment:
This change looks unrelated to the current PR. Please remove.
##########
test/e2e/predicates/predicates_test.go:
##########
@@ -423,7 +434,7 @@ var _ = Describe("Predicates", func() {
Ω(logEntries).To(ContainElement(MatchRegexp(".*taint.*")), "Log
entry message mismatch")
// Remove taint off the node and verify the pod is scheduled on
node.
- err = controller.RemoveTaintOffNode(context.Background(),
kClient.GetClient(), nodeName, nil, testTaint)
+ err = kClient.UntaintNode(nodeName, taintKey)
Review Comment:
This change looks unrelated to the current PR. Please remove.
##########
test/e2e/predicates/predicates_test.go:
##########
@@ -323,25 +329,28 @@ var _ = Describe("Predicates", func() {
It("Verify_Matching_Taint_Tolerations_Respected", func() {
nodeName := getNodeThatCanRunPodWithoutToleration(&kClient, ns)
By("Trying to apply a random taint on the found node.")
- testTaint := &v1.Taint{
- Key: fmt.Sprintf("kubernetes.io/e2e-taint-key-%s",
common.RandSeq(10)),
- Value: "testing-taint-value",
- Effect: v1.TaintEffectNoSchedule,
- }
- err = controller.AddOrUpdateTaintOnNode(context.Background(),
kClient.GetClient(), nodeName, testTaint)
+ taintKey := fmt.Sprintf("kubernetes.io/e2e-taint-key-%s",
common.RandSeq(10))
Review Comment:
This change looks unrelated to the current PR. Please remove.
##########
test/e2e/predicates/predicates_test.go:
##########
@@ -370,26 +379,28 @@ var _ = Describe("Predicates", func() {
It("Verify_Not_Matching_Taint_Tolerations_Respected", func() {
nodeName := getNodeThatCanRunPodWithoutToleration(&kClient, ns)
By("Trying to apply a random taint on the found node.")
- testTaint := &v1.Taint{
- Key: fmt.Sprintf("kubernetes.io/e2e-taint-key-%s",
common.RandSeq(10)),
- Value: "testing-taint-value",
- Effect: v1.TaintEffectNoSchedule,
- }
- err = controller.AddOrUpdateTaintOnNode(context.Background(),
kClient.GetClient(), nodeName, testTaint)
+ taintKey := fmt.Sprintf("kubernetes.io/e2e-taint-key-%s",
common.RandSeq(10))
Review Comment:
This change looks unrelated to the current PR. Please remove.
##########
test/e2e/predicates/predicates_test.go:
##########
@@ -995,9 +1012,15 @@ var _ = Describe("Predicates", func() {
By("Trying to apply a random label on the found node")
key := fmt.Sprintf("kubernetes.io/e2e-hostport-%s",
common.RandSeq(10))
value := "104"
- framework.AddOrUpdateLabelOnNode(kClient.GetClient(), nodeName,
key, value)
- framework.ExpectNodeHasLabel(kClient.GetClient(), nodeName,
key, value)
- defer framework.RemoveLabelOffNode(kClient.GetClient(),
nodeName, key)
+ err := kClient.SetNodeLabel(nodeName, key, value)
Review Comment:
This change looks unrelated to the current PR. Please remove.
##########
test/e2e/predicates/predicates_test.go:
##########
@@ -942,9 +953,15 @@ var _ = Describe("Predicates", func() {
By("Trying to apply a random label on the found node")
key := fmt.Sprintf("kubernetes.io/e2e-hostport-%s",
common.RandSeq(10))
value := "103"
- framework.AddOrUpdateLabelOnNode(kClient.GetClient(), nodeName,
key, value)
- framework.ExpectNodeHasLabel(kClient.GetClient(), nodeName,
key, value)
- defer framework.RemoveLabelOffNode(kClient.GetClient(),
nodeName, key)
+ err := kClient.SetNodeLabel(nodeName, key, value)
Review Comment:
This change looks unrelated to the current PR. Please remove.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
