Upgraded the dependent go modules #232

shriramsharma · 2022-06-22T00:27:30Z

Fixes #216
Upgraded the dependent go modules

Signed-off-by: Shriram Sharma shriram_sharma@intuit.com

codecov-commenter · 2022-06-23T23:12:18Z

Codecov Report

Merging #232 (7686693) into master (1d0fe30) will increase coverage by 0.00%.
The diff coverage is 74.68%.

@@           Coverage Diff           @@
##           master     #232   +/-   ##
=======================================
  Coverage   73.21%   73.21%           
=======================================
  Files          31       31           
  Lines        3084     3088    +4     
=======================================
+ Hits         2258     2261    +3     
- Misses        681      682    +1     
  Partials      145      145

Impacted Files	Coverage Δ
admiral/pkg/controller/admiral/service.go	`84.54% <36.36%> (+0.14%)`	⬆️
admiral/pkg/clusters/handler.go	`64.80% <51.72%> (ø)`
admiral/pkg/clusters/types.go	`58.21% <58.73%> (ø)`
admiral/pkg/controller/admiral/controller.go	`38.15% <60.00%> (-0.51%)`	⬇️
admiral/pkg/controller/admiral/configmap.go	`67.44% <70.00%> (ø)`
admiral/pkg/controller/admiral/deployment.go	`83.15% <84.61%> (ø)`
admiral/pkg/controller/admiral/rollouts.go	`75.47% <84.61%> (ø)`
admiral/pkg/clusters/serviceentry.go	`80.59% <85.71%> (-0.13%)`	⬇️
admiral/pkg/clusters/envoyfilter.go	`93.60% <95.00%> (+0.15%)`	⬆️
admiral/pkg/apis/admiral/routes/handlers.go	`68.86% <100.00%> (+0.29%)`	⬆️
... and 10 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

nirvanagit · 2022-06-24T16:52:27Z

admiral/pkg/client/clientset/versioned/clientset.go

 	if configShallowCopy.RateLimiter == nil && configShallowCopy.QPS > 0 {
 		if configShallowCopy.Burst <= 0 {
-			return nil, fmt.Errorf("Burst is required to be greater than 0 when RateLimiter is not set and QPS is set to greater than 0")
+			return nil, fmt.Errorf("burst is required to be greater than 0 when RateLimiter is not set and QPS is set to greater than 0")


nirvanagit · 2022-06-24T16:53:03Z

admiral/pkg/client/clientset/versioned/clientset.go

-	return &cs
+	cs, err := NewForConfig(c)
+	if err != nil {
+		panic(err)


log.Fatal instead?

admiral/pkg/clusters/handler_test.go

aattuluri

lgtm, thanks for doing this long pending work

nirvanagit · 2022-06-29T04:34:16Z

admiral/pkg/clusters/serviceentry_test.go


-	sidecarObj, _ := sidecarController.IstioClient.NetworkingV1alpha3().Sidecars("test-sidecar-namespace").Get(common.GetWorkloadSidecarName(), v12.GetOptions{})
+	sidecarObj, _ := sidecarController.IstioClient.NetworkingV1alpha3().Sidecars("test-sidecar-namespace").Get(ctx, common.GetWorkloadSidecarName(), v12.GetOptions{})


does this API return an error? If so we could add an assertion.

nirvanagit · 2022-06-29T04:34:34Z

admiral/pkg/clusters/serviceentry_test.go

@@ -534,15 +539,16 @@ func TestModifyExistingSidecarForLocalClusterCommunication(t *testing.T) {
 		Egress: []*istionetworkingv1alpha3.IstioEgressListener{&istioEgress},
 	}

-	createdSidecar, _ := sidecarController.IstioClient.NetworkingV1alpha3().Sidecars("test-sidecar-namespace").Create(existingSidecarObj)
+	ctx := context.Background()
+	createdSidecar, _ := sidecarController.IstioClient.NetworkingV1alpha3().Sidecars("test-sidecar-namespace").Create(ctx, existingSidecarObj, v12.CreateOptions{})


does this API return an error? If so we could add an assertion.

nirvanagit · 2022-06-29T04:38:50Z

admiral/pkg/controller/admiral/deployment_test.go

-	_, err = client.AppsV1().Deployments("test-ns").Create(&deploymentWithBadLabels)
-	_, err = client.AppsV1().Deployments("test-ns").Create(&deploymentWithIgnoreLabels)
+	_, err = client.AppsV1().Deployments("test-ns").Create(ctx, &deployment, metav1.CreateOptions{})
+	_, err = client.AppsV1().Deployments("test-ns").Create(ctx, &deploymentWithBadLabels, metav1.CreateOptions{})


Will be good to add assertions for each error.

nirvanagit · 2022-06-29T04:38:59Z

admiral/pkg/controller/admiral/deployment_test.go


 	if err != nil {
 		t.Errorf("%v", err)
 	}

 	depController.K8sClient = client
-	resultingDeps, _ := depController.GetDeployments()
+	resultingDeps, _ := depController.GetDeployments(ctx)


does this API return an error? If so we could add an assertion.

nirvanagit · 2022-06-29T04:40:56Z

admiral/pkg/test/resources/admins@fake-cluster.k8s.local

@@ -1,7 +1,7 @@
 apiVersion: v1
 clusters:
 - cluster:
-    certificate-authority-data: TUlJQ1VUQ0NBZnVnQXdJQkFnSUJBREFOQmdrcWhraUc5dzBCQVFRRkFEQlhNUXN3Q1FZRFZRUUdFd0pEVGpFTApNQWtHQTFVRUNCTUNVRTR4Q3pBSkJnTlZCQWNUQWtOT01Rc3dDUVlEVlFRS0V3SlBUakVMTUFrR0ExVUVDeE1DClZVNHhGREFTQmdOVkJBTVRDMGhsY205dVp5QlpZVzVuTUI0WERUQTFNRGN4TlRJeE1UazBOMW9YRFRBMU1EZ3gKTkRJeE1UazBOMW93VnpFTE1Ba0dBMVVFQmhNQ1EwNHhDekFKQmdOVkJBZ1RBbEJPTVFzd0NRWURWUVFIRXdKRApUakVMTUFrR0ExVUVDaE1DVDA0eEN6QUpCZ05WQkFzVEFsVk9NUlF3RWdZRFZRUURFd3RJWlhKdmJtY2dXV0Z1Clp6QmNNQTBHQ1NxR1NJYjNEUUVCQVFVQUEwc0FNRWdDUVFDcDVobkc3b2dCaHRseW5wT1MyMWNCZXdLRS9CN2oKVjE0cWV5c2xucjI2eFpVc1NWa28zNlpuaGlhTy96Yk1Pb1JjS0s5dkVjZ010Y0xGdVFUV0RsM1JBZ01CQUFHagpnYkV3Z2E0d0hRWURWUjBPQkJZRUZGWEk3MGtyWGVRRHhaZ2JhQ1FvUjRqVURuY0VNSDhHQTFVZEl3UjRNSGFBCkZGWEk3MGtyWGVRRHhaZ2JhQ1FvUjRqVURuY0VvVnVrV1RCWE1Rc3dDUVlEVlFRR0V3SkRUakVMTUFrR0ExVUUKQ0JNQ1VFNHhDekFKQmdOVkJBY1RBa05PTVFzd0NRWURWUVFLRXdKUFRqRUxNQWtHQTFVRUN4TUNWVTR4RkRBUwpCZ05WQkFNVEMwaGxjbTl1WnlCWllXNW5nZ0VBTUF3R0ExVWRFd1FGTUFNQkFmOHdEUVlKS29aSWh2Y05BUUVFCkJRQURRUUEvdWd6QnJqaks5amNXbkRWZkdIbGszaWNOUnEwb1Y3UmkzMnovK0hRWDY3YVJmZ1p1N0tXZEkrSnUKV203RENmclBOR1Z3RldVUU9tc1B1ZTlyWkJnTw==
+    certificate-authority-data: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tDQpNSUlFN3pDQ0ExZWdBd0lCQWdJUU5MbUh5bU9wUy9Ib3Y2bFdyT3hvU1RBTkJna3Foa2lHOXcwQkFRc0ZBRENCDQpqekVlTUJ3R0ExVUVDaE1WYld0alpYSjBJR1JsZG1Wc2IzQnRaVzUwSUVOQk1USXdNQVlEVlFRTERDbHpjMmhoDQpjbTFoTXpSQWFXNTBkV2wwWkdWd1lqRXlaV0VnS0ZOb2NtbHlZVzBnVTJoaGNtMWhLVEU1TURjR0ExVUVBd3d3DQpiV3RqWlhKMElITnphR0Z5YldFek5FQnBiblIxYVhSa1pYQmlNVEpsWVNBb1UyaHlhWEpoYlNCVGFHRnliV0VwDQpNQjRYRFRJeE1EZ3dNakl5TXpJek9Gb1hEVE14TURnd01qSXlNekl6T0Zvd2dZOHhIakFjQmdOVkJBb1RGVzFyDQpZMlZ5ZENCa1pYWmxiRzl3YldWdWRDQkRRVEV5TURBR0ExVUVDd3dwYzNOb1lYSnRZVE0wUUdsdWRIVnBkR1JsDQpjR0l4TW1WaElDaFRhSEpwY21GdElGTm9ZWEp0WVNreE9UQTNCZ05WQkFNTU1HMXJZMlZ5ZENCemMyaGhjbTFoDQpNelJBYVc1MGRXbDBaR1Z3WWpFeVpXRWdLRk5vY21seVlXMGdVMmhoY20xaEtUQ0NBYUl3RFFZSktvWklodmNODQpBUUVCQlFBRGdnR1BBRENDQVlvQ2dnR0JBTWM1eHBwOCtHSmxCbGVTYTc0L0VnR3J4a3VZd3M0Mmd6UzRWS2xwDQpiVTNRSXpDV2psOVd4L0hhRVVqLzA3R1dtMlhOQWVoSS91b1JIS0x0aERMWWdMelRoazBMTDFqZkZCaDV3bDRtDQplaDlCcE5EUnhXcDNQaWhScW13SGUxNFArbDR1YU1aSHNveG5WSncvQS9La05pZXhMVXZMOVZqUXpacko4Y1luDQovTjZWVjNuc1dGQjF5dVlCdHBJUHo2ODJQS291cHpqcTVRcWdOUmo1dzFHM2p0MUFabjViK004YmNKMlQvUG1hDQp4UkNVa3NxTWpwdjZhVVdpdkd1UW5yL0xaVWpxWWhSU2xLYlNoOGdjRjNmZ3VSOC9ZTjB1OVM0R1lEa2JwMXc5DQpRYitwYk5CeS9pSkdkSzJOS0JsOHRvQmdESWx2OFlmSmdEbDdCbk1uM1J2NG02T0w0VHVVVjIva25JKy9xMkZsDQpTWW8rL0F0QUVtRGV2ZEIxbUV3emRId1AvR0NsT2QrV1hjSEV2TENibC9MZ2FIeC8zZWUyNmEwb1hzVlp4N21PDQpXMGxrMU1JTXVlbTdjMTh6WXdjRDlzUXBVVG4xRTcrTGlzWHpVNC9SdVB6MHFpQUp5RzRVZ3lqY0JzdVZ6UTNqDQo2WUNaWlUxb2ZLVzQ1aWF1QmpaOUNKS2ZVUUlEQVFBQm8wVXdRekFPQmdOVkhROEJBZjhFQkFNQ0FnUXdFZ1lEDQpWUjBUQVFIL0JBZ3dCZ0VCL3dJQkFEQWRCZ05WSFE0RUZnUVVacVdGS1BrY3d4cWFOejUvbDBhZzNYMWpGNzh3DQpEUVlKS29aSWh2Y05BUUVMQlFBRGdnR0JBSlYrRE5DanViZUdqejgyQ1FFMmNSbyszektDSm42T3pTaE85WnVCDQpFM3dxUFBJT1QvOTBtZm40dEJhT0R0NmovNi9mRTZad1YyWU93UVErR3plOGZORzJEMEZYVE16a3l3Q0VwcEkyDQpIWmtIZ0JPOE45VFhja0pIR1crYkFXc0lSRHc2V2pvbThQWTN1RDA1QWEzV2RYd2ROYUE1R2hTQ3ZVVEU0bVVYDQpESjdYVjVLZjlPdEhObllXOFZ3b3B0NlBhM2tpUHdQZE9BU0E1OVUvVVFhWTRwVE5TRnBTY1A1R1VEbnVKUTUwDQpXOEdmem1FNkV4Y0ROOXRMRnVYT0oyelFpTWpKMGVKQUxIckM2UUtrRXk0anpCTFY0RFN0SURBQ2dqbzdtU28zDQphWER0NnozbTVlSUp4TmIrMG51VTRnc1BRbnQwb2sxVW9VNVpES3ZRZ3FKcGZSMU9NYUlEWkVPREJ6dXpZYTE2DQp1V0pOQ3BEaU4rZGtBMXhQQ08wVUk4STRqcDBxek9rMjNzejlEVEwvNldNbldndkZRdWxJcVRWMDNHVE1MRnEyDQpFOXpLb0JqQzdURUJrczVwSlZmMkhwVkdrUitMVzVnZnBVT0ZFanFLYnBhMEorV1hHVVRSU3R1UVBiNjQzRVBTDQpZeEl6TGR4OW04RU1zeHpYTjNQUGpDVTYxdz09DQotLS0tLUVORCBDRVJUSUZJQ0FURS0tLS0tDQo=


Just curious, why was an update required to ca data?

somehow I was getting the following error so I updated it.

destinationrule_test.go:28: Unexpected err failed to create destination rule controller k8s client: unable to load root certificates: unable to parse bytes as PEM block destinationrule_test.go:32: DestinationRule controller should never be nil without an error thrown --- FAIL: TestNewDestinationRuleController

Cool, Just ran openssl command after decoding using base64, and the old one was not a certificate.

nirvanagit · 2022-06-29T04:42:54Z

admiral/pkg/clusters/handler_test.go

@@ -1479,7 +1486,7 @@ func TestAddUpdateServiceEntry(t *testing.T) {
 		Spec:       twoEndpointSe,
 	}

-	seCtrl.IstioClient.NetworkingV1alpha3().ServiceEntries("namespace").Create(oldSeTwoEndpoints)
+	seCtrl.IstioClient.NetworkingV1alpha3().ServiceEntries("namespace").Create(ctx, oldSeTwoEndpoints, v12.CreateOptions{})


should we check for error in this?

nirvanagit · 2022-06-29T04:43:11Z

admiral/pkg/clusters/handler_test.go

 			if c.skipDestructive {
 				//verify the update did not go through
-				se, _ := c.rc.ServiceEntryController.IstioClient.NetworkingV1alpha3().ServiceEntries("namespace").Get(c.oldSe.Name, v12.GetOptions{})
+				se, _ := c.rc.ServiceEntryController.IstioClient.NetworkingV1alpha3().ServiceEntries("namespace").Get(ctx, c.oldSe.Name, v12.GetOptions{})


does this API return an error? If so we could add an assertion.

nirvanagit · 2022-06-29T04:57:50Z

admiral/pkg/client/clientset/versioned/clientset.go

-	return &cs
+	cs, err := NewForConfig(c)
+	if err != nil {
+		log.Fatal(err)


Sorry for the second comment on the same line. I think we should return an error to the caller.

shriramsharma · 2022-07-01T18:24:41Z

admiral/pkg/clusters/handler.go

 		op = "Add"
 	} else {
-		exist.Labels = obj.Labels
-		exist.Annotations = obj.Annotations
-		exist.Spec = obj.Spec


hey @aattuluri , we were getting copylock linting errors on this line and @nirvanagit and I quickly scanned through the code and it seemed we could pass obj directly to the Update func without having to assign it to exist. Since we don't have much historic context to it, can you please review this and let us know if this is ok?

In this case obj is not something returned by the client but rather a new generated one. I think it might work. I am fine as long as the tests pass. Dont really remember why it was written this way :)

shriramsharma · 2022-07-01T18:40:28Z

admiral/pkg/clusters/handler.go

@@ -599,8 +599,7 @@ func addUpdateServiceEntry(obj *v1alpha3.ServiceEntry, exist *v1alpha3.ServiceEn
 			log.Infof(LogFormat, op, "ServiceEntry", obj.Name, rc.ClusterID, "Update skipped as it was destructive during Admiral's bootup phase")
 			return
 		} else {
-			exist.Spec = obj.Spec


@aattuluri , here is the same as before for resolving copylock

shriramsharma · 2022-07-01T18:40:50Z

admiral/pkg/clusters/handler.go

 		op = "Add"
 	} else {
-		exist.Labels = obj.Labels
-		exist.Annotations = obj.Annotations
-		exist.Spec = obj.Spec


@aattuluri , here is the same as before for resolving copylock

shriramsharma · 2022-07-01T18:41:30Z

admiral/pkg/clusters/serviceentry.go

@@ -368,10 +368,7 @@ func modifySidecarForLocalClusterCommunication(ctx context.Context, sidecarNames

 func addUpdateSidecar(ctx context.Context, obj *v1alpha3.Sidecar, exist *v1alpha3.Sidecar, namespace string, rc *RemoteController) {
 	var err error
-	exist.Labels = obj.Labels
-	exist.Annotations = obj.Annotations
-	exist.Spec = obj.Spec


@aattuluri , here is the same as before for resolving copylock

aattuluri

This looks good. Do we need to generate a client/api for dependency record also?
Also, how was this generated. Would be good to add a command to Makefile to generate this in future.

Upgraded the dependent go modules Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>

Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>

aattuluri

lgtm

Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>

Signed-off-by: Anubhav Aeron <anubhav_aeron@intuit.com>

shriramsharma added the WIP Work In Progress label Jun 22, 2022

shriramsharma requested review from nirvanagit and aattuluri June 22, 2022 00:27

nirvanagit reviewed Jun 24, 2022

View reviewed changes

admiral/pkg/clusters/handler_test.go Show resolved Hide resolved

aattuluri approved these changes Jun 27, 2022

View reviewed changes

shriramsharma force-pushed the upgrade_deps_216 branch from 1c87baa to cb9b3c5 Compare June 28, 2022 21:35

nirvanagit reviewed Jun 29, 2022

View reviewed changes

shriramsharma commented Jul 1, 2022

View reviewed changes

shriramsharma added do not merge and removed WIP Work In Progress labels Jul 7, 2022

aattuluri mentioned this pull request Jul 25, 2022

Update CRD to use apiextensions.k8s.io/v1 #247

Closed

shriramsharma force-pushed the upgrade_deps_216 branch from 97eaec6 to e26473f Compare July 26, 2022 05:17

aattuluri requested changes Jul 26, 2022

View reviewed changes

shriramsharma added 4 commits August 14, 2022 23:00

Fixes istio-ecosystem#216

b7b7b0b

Upgraded the dependent go modules Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>

made fixes to work with go1.17

55f0397

Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>

updated the golang version in circleci config

b1c4c48

Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>

fixed the linting errors

9ca9a56

Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>

shriramsharma added 8 commits August 14, 2022 23:31

fixed linting issues

a2a94f8

Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>

tried to fix the linting errors

f351354

Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>

fixed as per review comments

a519fee

Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>

fixed as per review comments

0c11859

Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>

fixed linting errors

cd7b252

Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>

added GTP client API

241baf9

Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>

removed the redundant gtp client

83a39a9

Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>

rebased and fixed as needed

d305f4c

Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>

shriramsharma force-pushed the upgrade_deps_216 branch from 3ada347 to d305f4c Compare August 15, 2022 19:25

shriramsharma added 2 commits August 15, 2022 13:17

fixed linting errors

0b0debe

Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>

upgraded the go version tp 1.18

3106ad4

Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>

shriramsharma requested review from nirvanagit and aattuluri August 17, 2022 04:01

aattuluri approved these changes Aug 17, 2022

View reviewed changes

nirvanagit approved these changes Aug 17, 2022

View reviewed changes

fixed as per review comments

7686693

Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>

shriramsharma removed the do not merge label Aug 17, 2022

shriramsharma merged commit bdfcbae into istio-ecosystem:master Aug 17, 2022

shriramsharma deleted the upgrade_deps_216 branch August 17, 2022 22:53

nirvanagit pushed a commit that referenced this pull request Sep 19, 2022

Upgraded the dependent go modules (#232)

dde7448

Signed-off-by: Anubhav Aeron <anubhav_aeron@intuit.com>

itsLucario pushed a commit that referenced this pull request Nov 7, 2022

Upgraded the dependent go modules (#232) (#161)

c06b5ee

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgraded the dependent go modules #232

Upgraded the dependent go modules #232

shriramsharma commented Jun 22, 2022

codecov-commenter commented Jun 23, 2022 •

edited

Loading

nirvanagit Jun 24, 2022

nirvanagit Jun 24, 2022

aattuluri left a comment

nirvanagit Jun 29, 2022 •

edited

Loading

nirvanagit Jun 29, 2022

nirvanagit Jun 29, 2022

nirvanagit Jun 29, 2022

nirvanagit Jun 29, 2022

shriramsharma Jun 30, 2022

nirvanagit Aug 17, 2022

nirvanagit Jun 29, 2022

nirvanagit Jun 29, 2022

nirvanagit Jun 29, 2022

shriramsharma Jul 1, 2022

aattuluri Jul 1, 2022

shriramsharma Jul 1, 2022

shriramsharma Jul 1, 2022

shriramsharma Jul 1, 2022

aattuluri left a comment

aattuluri left a comment


		sidecarObj, _ := sidecarController.IstioClient.NetworkingV1alpha3().Sidecars("test-sidecar-namespace").Get(common.GetWorkloadSidecarName(), v12.GetOptions{})
		sidecarObj, _ := sidecarController.IstioClient.NetworkingV1alpha3().Sidecars("test-sidecar-namespace").Get(ctx, common.GetWorkloadSidecarName(), v12.GetOptions{})

Upgraded the dependent go modules #232

Upgraded the dependent go modules #232

Conversation

shriramsharma commented Jun 22, 2022

codecov-commenter commented Jun 23, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aattuluri left a comment

Choose a reason for hiding this comment

nirvanagit Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aattuluri left a comment

Choose a reason for hiding this comment

aattuluri left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 23, 2022 •

edited

Loading

nirvanagit Jun 29, 2022 •

edited

Loading