Add support to configure `EventInvokeConfig` for `Version` #115

Vandita2020 · 2023-10-26T18:37:59Z

Issue #, if available:

Description of changes:

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

ack-prow · 2023-10-26T18:38:02Z

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

a-hilaly

Looks good! I have two questions below

a-hilaly · 2024-01-11T21:11:50Z

apis/v1alpha1/version.go

@@ -32,7 +32,8 @@ type VersionSpec struct {
 	CodeSHA256 *string `json:"codeSHA256,omitempty"`
 	// A description for the version to override the description in the function
 	// configuration.
-	Description *string `json:"description,omitempty"`
+	Description               *string                            `json:"description,omitempty"`
+	FunctionEventInvokeConfig *PutFunctionEventInvokeConfigInput `json:"functionEventInvokeConfig,omitempty"`


Thoughts: how about naming this field EventInvokeConfig - how does CF support this?

Good suggestion, the CF uses the same naming too EventInvokeConfig. Here's the link

Looks like it's named FunctionEventInvokeConfig in https://github.com/aws-controllers-k8s/lambda-controller/blob/main/apis/v1alpha1/function.go#L66

To keep the things similar across all resources, I am keeping the naming to be FunctionEventInvokeConfig. Will open a PR in future to correct the naming for all resources.

a-hilaly · 2024-01-11T21:13:03Z

pkg/resource/version/hooks.go

+func (rm *resourceManager) customUpdateVersion(
+	ctx context.Context,
+	desired *resource,
+	latest *resource,
+	delta *ackcompare.Delta,
+) (*resource, error) {
+	var err error
+	rlog := ackrtlog.FromContext(ctx)
+	exit := rlog.Trace("rm.customUpdateFunction")
+	defer exit(err)
+
+	if isVersionPending(desired) {
+		return nil, requeueWaitWhilePending
+	}
+
+	if delta.DifferentAt("Spec.FunctionEventInvokeConfig") {
+		err = rm.syncEventInvokeConfig(ctx, desired)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	readOneLatest, err := rm.ReadOne(ctx, desired)
+	if err != nil {
+		return nil, err
+	}
+	return rm.concreteResource(readOneLatest), nil
+}


What if it's the SHA256 or revisionID that changed?

SHA256 and revisionID needs to be matched with the code, if they are different than the expected ones, the controller will not publish version and will throw an error. So those fields cannot be changed and even if one change them and they do not match with the expected, the API will return an error

a-hilaly · 2024-01-11T21:17:54Z

test/e2e/tests/test_version.py

+        lambda_validator = LambdaValidator(lambda_client)
+
+        version_number = cr['status']['version']


Can we assert that the version is 1 ? or 2?

Maybe more in the test_smoke function

I am not sure if that would work or fail, giving it a try by hard coding version to be 1 for test_smoke function. My guess is while running tests if the controller calls the same API twice for publishing version, the second time the Version won't be published as the same version_number (which is 1) will be passed.

I tried keeping the version to be 1, but the tests failed. My guess is this is because controller might be using same function and publishing Version for it and then deleting it. The concept is even after deleting the function and its associated Version, when it creates the Function with same name and tries to publish Version for it, the Version number gets incremented from the last published version. Thus we cannot hard bound the Version number, instead we're getting the Version number after the Version is published and then using it to check if the Version exists.

Thanks for the clarification. QQ: Why not use different tests functions with brand new names so that we make sure that the published version is 1. Asserting that the versions are 1 is a mandatory thing for us to do, before making a release.

Added a new test function named test_smoke_version_specified keeping Version to be 1 and testing.

a-hilaly · 2024-01-14T20:09:57Z

/test all

a-hilaly · 2024-01-15T02:42:56Z

/hold

a-hilaly · 2024-01-26T06:22:56Z

/test all

Create Version resource Delete functionality mend Check code changes before creating new version resource Adding E2E tests Review changes Add support to configure EventInvokeConfig for Version E2E tests for EventInvokeConfig

a-hilaly · 2024-01-31T00:48:04Z

/test all

a-hilaly · 2024-01-31T01:00:43Z

/retest

a-hilaly · 2024-01-31T01:02:29Z

/test all

a-hilaly · 2024-01-31T01:19:55Z

/retest

a-hilaly · 2024-01-31T16:00:34Z

/retest

test/e2e/tests/test_version.py

valerena · 2024-01-31T18:58:59Z

test/e2e/tests/test_version.py

@@ -81,7 +81,9 @@ def lambda_function():
 @pytest.mark.canary
 class TestVersion:
    def test_smoke(self, lambda_client, lambda_function):
-        (_, function_resource) = lambda_function
+        (function_reference, function_resource) = lambda_function
+        fn_resource = k8s.get_resource(function_reference)


Is this fn_resource actually different than function_resource? Any reason to not just use the same variable?

Thanks for bringing this up. Have kept fn_resource and used it. Need to use fn_resource as k8s.get_resource is passing the whole function resource which can then be modified, which is required in our case.

a-hilaly

Thank you @Vandita2020 !
/lgtm

ack-prow · 2024-02-01T21:07:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, 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~~ [Vandita2020,a-hilaly]

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

a-hilaly · 2024-02-01T21:07:28Z

/unhold

ack-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2023

ack-prow bot requested review from a-hilaly and RedbackThomson October 26, 2023 18:38

ack-prow bot added the approved label Oct 26, 2023

Vandita2020 marked this pull request as ready for review October 30, 2023 21:14

ack-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2023

ack-prow bot requested a review from jljaco October 30, 2023 21:14

Vandita2020 force-pushed the functionVersionConfig branch 2 times, most recently from 3c7e8b4 to 3b15fe8 Compare January 11, 2024 21:09

a-hilaly reviewed Jan 11, 2024

View reviewed changes

a-hilaly removed the request for review from RedbackThomson January 11, 2024 21:20

Vandita2020 force-pushed the functionVersionConfig branch 3 times, most recently from 65170b8 to 9808122 Compare January 11, 2024 22:45

ack-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 15, 2024

Vandita2020 force-pushed the functionVersionConfig branch 2 times, most recently from 7b48238 to 10bc8a0 Compare January 17, 2024 19:08

Vandita2020 force-pushed the functionVersionConfig branch 3 times, most recently from b3bf2c1 to 44403ae Compare January 24, 2024 22:41

aws-controllers-k8s deleted a comment from ack-prow bot Jan 24, 2024

Vandita2020 force-pushed the functionVersionConfig branch from 44403ae to 97dad94 Compare January 24, 2024 23:03

aws-controllers-k8s deleted a comment from ack-prow bot Jan 24, 2024

Vandita2020 force-pushed the functionVersionConfig branch from 97dad94 to 6a71d99 Compare January 24, 2024 23:23

aws-controllers-k8s deleted a comment from ack-prow bot Jan 24, 2024

Vandita2020 force-pushed the functionVersionConfig branch from 6a71d99 to 79df778 Compare January 24, 2024 23:48

aws-controllers-k8s deleted a comment from ack-prow bot Jan 24, 2024

Vandita2020 force-pushed the functionVersionConfig branch from 79df778 to 74af390 Compare January 25, 2024 00:14

aws-controllers-k8s deleted a comment from ack-prow bot Jan 25, 2024

Vandita2020 force-pushed the functionVersionConfig branch from 74af390 to 81f8f2f Compare January 25, 2024 21:15

aws-controllers-k8s deleted a comment from ack-prow bot Jan 25, 2024

Vandita2020 force-pushed the functionVersionConfig branch from 81f8f2f to 020045d Compare January 25, 2024 21:49

aws-controllers-k8s deleted a comment from ack-prow bot Jan 25, 2024

Add support to configure EventInvokeConfig for Version

47abd66

Create Version resource Delete functionality mend Check code changes before creating new version resource Adding E2E tests Review changes Add support to configure EventInvokeConfig for Version E2E tests for EventInvokeConfig

Vandita2020 force-pushed the functionVersionConfig branch from 020045d to 3441bf0 Compare January 31, 2024 00:04

valerena reviewed Jan 31, 2024

View reviewed changes

Vandita2020 force-pushed the functionVersionConfig branch 3 times, most recently from c0661ca to a7e13c6 Compare February 1, 2024 19:09

aws-controllers-k8s deleted a comment from ack-prow bot Feb 1, 2024

Vandita2020 force-pushed the functionVersionConfig branch from a7e13c6 to c30edd1 Compare February 1, 2024 20:46

second approach

13dfbef

Vandita2020 force-pushed the functionVersionConfig branch from c30edd1 to 13dfbef Compare February 1, 2024 21:00

a-hilaly approved these changes Feb 1, 2024

View reviewed changes

ack-prow bot assigned a-hilaly Feb 1, 2024

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2024

ack-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2024

ack-prow bot merged commit e9b9621 into aws-controllers-k8s:main Feb 1, 2024

		lambda_validator = LambdaValidator(lambda_client)

		version_number = cr['status']['version']

Add support to configure EventInvokeConfig for Version #115

Add support to configure EventInvokeConfig for Version #115

Uh oh!

Conversation

Vandita2020 commented Oct 26, 2023

Uh oh!

ack-prow bot commented Oct 26, 2023

Uh oh!

a-hilaly left a comment

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!

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 commented Jan 14, 2024

Uh oh!

a-hilaly commented Jan 15, 2024

Uh oh!

a-hilaly commented Jan 26, 2024

Uh oh!

a-hilaly commented Jan 31, 2024

Uh oh!

a-hilaly commented Jan 31, 2024

Uh oh!

a-hilaly commented Jan 31, 2024

Uh oh!

a-hilaly commented Jan 31, 2024

Uh oh!

a-hilaly commented Jan 31, 2024

Uh oh!

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!

ack-prow bot commented Feb 1, 2024

Uh oh!

a-hilaly commented Feb 1, 2024

Uh oh!

Uh oh!

Add support to configure `EventInvokeConfig` for `Version` #115

Add support to configure `EventInvokeConfig` for `Version` #115