Add e2e tests for `LayerVersion` resource update operation #68

Vandita2020 · 2022-12-06T22:57:10Z

Description of changes:
Add e2e tests for LayerVersion resource update operation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-bot · 2022-12-06T22:57:12Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Vandita2020 · 2022-12-06T22:57:18Z

/test all

Vandita2020 · 2022-12-06T23:34:20Z

/test all

jaypipes

@Vandita2020 the PR summary says you are just updating e2e tests for layer versions, however I see some functional changes in this PR (to delta.go and sdk.go) which indicates this is more than just a change to e2e tests :) Can you either update the PR summary and description to incorporate those functional changes or remove the functional changes from this PR?

Thank you!
-jay

Vandita2020 · 2022-12-07T23:03:05Z

/test all

Vandita2020 · 2022-12-07T23:05:35Z

@Vandita2020 the PR summary says you are just updating e2e tests for layer versions, however I see some functional changes in this PR (to delta.go and sdk.go) which indicates this is more than just a change to e2e tests :) Can you either update the PR summary and description to incorporate those functional changes or remove the functional changes from this PR?

Thank you! -jay

Hey @jaypipes,
Thanks for bringing this to my notice, I have made the changes. Now this PR only contains update e2e tests for Layer_version resource.

Vandita2020 · 2022-12-08T00:26:32Z

/test all

jaypipes · 2022-12-08T16:32:41Z

test/e2e/tests/test_layer_version.py

@@ -79,6 +79,19 @@ def test_smoke(self, lambda_client):
        # Check layer version exists
        assert lambda_validator.layer_version_exists(resource_name, version_number)

+        logging.info(cr['spec']['description'])
+        # Update cr
+        cr['spec']['description'] = 'new description'


@Vandita2020 Instead of passing in an updated cr object, try just passing in the things you want to update.

Follow this example which shows you how to do this:

https://github.com/aws-controllers-k8s/iam-controller/blob/f528ed16a631d2b57f4fd06eb1abc1dac08ea099/test/e2e/tests/test_role.py#L99-L105

(I agree it's a bit weird! :) )

@jaypipes Thanks so much for sharing this, I was really looking for another way to update the fields.

Vandita2020 · 2022-12-08T21:00:20Z

/test all

Vandita2020 · 2022-12-08T21:15:11Z

/test all

Vandita2020 · 2022-12-08T21:15:22Z

/test all

Vandita2020 · 2022-12-12T23:47:35Z

/test all

a-hilaly · 2022-12-13T00:09:05Z

config/crd/common/bases/services.k8s.aws_fieldexports.yaml

 ---
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
  annotations:
-    controller-gen.kubebuilder.io/version: v0.9.2
+    controller-gen.kubebuilder.io/version: v0.7.0


You need to install codegen v0.9.2

a-hilaly · 2022-12-13T00:09:17Z

test/e2e/tests/test_layer_version.py

+        cr = k8s.wait_resource_consumed_by_controller(ref)
+        version_number = cr['status']['versionNumber']


controller-gen v0.9.0 needs to be used to regenerate the controller - also we need to keep the current tag. /hold

Done

a-hilaly · 2022-12-13T00:10:46Z

config/controller/kustomization.yaml

@@ -6,4 +6,4 @@ kind: Kustomization
 images:
 - name: controller
  newName: public.ecr.aws/aws-controllers-k8s/lambda-controller
-  newTag: v0.1.4
+  newTag: v0.1.3


and pull the all the repository tags

a-hilaly · 2022-12-13T00:11:54Z

test/e2e/resources/layer_version.yaml

+  name: v1
+  annotations: 
+    services.k8s.aws/region: us-west-2
 spec:
-  layerName: $LAYER_VERSION
+  layerName: v1
  content:
-    s3Bucket: $BUCKET_NAME
-    s3Key: $LAMBDA_FILE_NAME
+    s3Bucket: 322962841005-lambda-test
+    s3Key: my-deployment-package.zip


I think the reason e2e tests are failing is that you forgot to undo the changes made to this file.

Done. Thanks

Vandita2020 · 2022-12-13T00:17:29Z

/test all

a-hilaly · 2022-12-13T15:42:07Z

controller-gen v0.9.0 needs to be used to regenerate the controller - also we need to keep the current tag.
/hold

Vandita2020 · 2022-12-13T21:00:13Z

/test all

a-hilaly · 2022-12-13T21:45:22Z

go.mod

+	k8s.io/api v0.24.0
+	k8s.io/apimachinery v0.24.0
+	k8s.io/client-go v0.24.0


Please revert the changes made in go.mod and go.sum files.

a-hilaly · 2022-12-13T21:45:37Z

apis/v1alpha1/ack-generate-metadata.yaml

+  build_date: "2022-12-13T20:52:55Z"
+  build_hash: 39d7c30b8df3c44a9804f578cd6acc9a3f669b03
+  go_version: go1.19
+  version: v0.20.1-15-g39d7c30-dirty


nit: make sure that you're generating the controller with a non dirty version

a-hilaly · 2022-12-13T21:46:09Z

test/e2e/resources/layer_version.yaml

@@ -2,7 +2,7 @@ apiVersion: lambda.services.k8s.aws/v1alpha1
 kind: LayerVersion
 metadata:
  name: $LAYER_VERSION
-  annotations:
+  annotations: 


nit: unnecessary space

a-hilaly · 2022-12-13T21:46:33Z

test/e2e/tests/test_layer_version.py

+CREATE_WAIT_AFTER_SECONDS = 30
+UPDATE_WAIT_AFTER_SECONDS = 30
+DELETE_WAIT_AFTER_SECONDS = 30


Now that you figured out what was the issue with layer_version tests, shall we reduce those values to 10?

a-hilaly

Thank you, @Vandita2020 !
/lgtm

jaypipes

👍 thanks @Vandita2020!

jaypipes · 2022-12-15T12:59:34Z

/lgtm

ack-bot · 2022-12-15T12:59:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, jaypipes, Vandita2020

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:

~~OWNERS~~ [A-Hilaly,jaypipes]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ack-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 6, 2022

ack-bot requested review from a-hilaly and vijtrip2 December 6, 2022 22:57

Vandita2020 force-pushed the layer_version_e2e_tests_update branch from 70973a1 to e63af8b Compare December 6, 2022 23:34

jaypipes suggested changes Dec 7, 2022

View reviewed changes

Vandita2020 force-pushed the layer_version_e2e_tests_update branch from e63af8b to 001512e Compare December 7, 2022 23:02

Vandita2020 force-pushed the layer_version_e2e_tests_update branch from 001512e to 4df5b40 Compare December 8, 2022 00:26

jaypipes reviewed Dec 8, 2022

View reviewed changes

Vandita2020 force-pushed the layer_version_e2e_tests_update branch from 4df5b40 to e3cc9db Compare December 8, 2022 21:00

Vandita2020 force-pushed the layer_version_e2e_tests_update branch from e3cc9db to 37b5b15 Compare December 8, 2022 21:14

Vandita2020 force-pushed the layer_version_e2e_tests_update branch from 37b5b15 to 245d0f8 Compare December 12, 2022 23:47

a-hilaly reviewed Dec 13, 2022

View reviewed changes

Vandita2020 force-pushed the layer_version_e2e_tests_update branch from 245d0f8 to 7875344 Compare December 13, 2022 00:17

Vandita2020 marked this pull request as ready for review December 13, 2022 00:27

ack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 13, 2022

Vandita2020 changed the title ~~Draft: Adding update e2e tests for Layer_version resource~~ Adding update e2e tests for Layer_version resource Dec 13, 2022

a-hilaly changed the title ~~Adding update e2e tests for Layer_version resource~~ Add e2e tests for LayerVersion resource update operation Dec 13, 2022

ack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 13, 2022

Vandita2020 force-pushed the layer_version_e2e_tests_update branch from 7875344 to 258d338 Compare December 13, 2022 20:59

a-hilaly reviewed Dec 13, 2022

View reviewed changes

Draft: Adding update e2e tests for Layer_version resource

b5aaced

Vandita2020 force-pushed the layer_version_e2e_tests_update branch from 258d338 to b5aaced Compare December 13, 2022 23:11

a-hilaly approved these changes Dec 14, 2022

View reviewed changes

ack-bot assigned a-hilaly Dec 14, 2022

ack-bot added lgtm Indicates that a PR is ready to be merged. approved labels Dec 14, 2022

jaypipes approved these changes Dec 15, 2022

View reviewed changes

jaypipes removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2022

ack-bot assigned jaypipes Dec 15, 2022

ack-bot merged commit 2771d6a into aws-controllers-k8s:main Dec 15, 2022

		cr = k8s.wait_resource_consumed_by_controller(ref)
		version_number = cr['status']['versionNumber']

Add e2e tests for LayerVersion resource update operation #68

Add e2e tests for LayerVersion resource update operation #68

Uh oh!

Conversation

Vandita2020 commented Dec 6, 2022 • edited by a-hilaly Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ack-bot commented Dec 6, 2022

Uh oh!

Vandita2020 commented Dec 6, 2022

Uh oh!

Vandita2020 commented Dec 6, 2022

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

Vandita2020 commented Dec 7, 2022

Uh oh!

Vandita2020 commented Dec 7, 2022

Uh oh!

Vandita2020 commented Dec 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vandita2020 commented Dec 8, 2022

Uh oh!

Vandita2020 commented Dec 8, 2022

Uh oh!

Vandita2020 commented Dec 8, 2022

Uh oh!

Vandita2020 commented Dec 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vandita2020 commented Dec 13, 2022

Uh oh!

a-hilaly commented Dec 13, 2022

Uh oh!

Vandita2020 commented Dec 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

jaypipes commented Dec 15, 2022

Uh oh!

Add e2e tests for `LayerVersion` resource update operation #68

Add e2e tests for `LayerVersion` resource update operation #68

Vandita2020 commented Dec 6, 2022 •

edited by a-hilaly

Loading