Support of injecting fields in existing structures: CustomNestedFields #462

Vandita2020 · 2023-08-23T23:23:16Z

Issue #
This issue arises from the code changes in lambda-controller for PR #88

Summary
Support of injecting fields in existing structures: CustomNestedFields

Description
The code in this PR helps in accepting a custom nestedField defined using type as a part of its parentField. In the issue mentioned above when we inject a new field S3SHA256 into Code struct, instead of nesting it under Code the generator created a separate field named CodeS3SHA256.

The new field S3SHA256 is defined like this in generator.yaml file:

Function:
    fields:
      Code.S3SHA256:
        type: string

After applying the changes in this PR, the field S3SHA256 is properly injected as a part of Code struct.

type FunctionCode struct {
	ImageURI *string `json:"imageURI,omitempty"`
	S3Bucket *string `json:"s3Bucket,omitempty"`
	S3BucketRef     *ackv1alpha1.AWSResourceReferenceWrapper `json:"s3BucketRef,omitempty"`
	S3Key           *string                                  `json:"s3Key,omitempty"`
	S3ObjectVersion *string                                  `json:"s3ObjectVersion,omitempty"`
	S3SHA256        *string                                  `json:"s3SHA256,omitempty"`
	ZipFile         []byte                                   `json:"zipFile,omitempty"`
}

This PR contains code implementation to inject Nested fields into the Spec or Status struct and has respective model-test to validate the changes.

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

a-hilaly

@Vandita2020 This is great work, well done!
I left a few comments to improve the code and make it more similar to what we have in the other packages.

a-hilaly · 2023-08-24T21:43:19Z

.vscode/settings.json

+{
+    "go.buildTags": "codegen",
+}


Do not commit this please. Maybe let's add .vscode/settings.json to .gitignore file ?

I'd even recommend adding .vscode to a global gitignore on your system. Otherwise every repo needs to add every IDE's config paths (.vscode, .idea, etc.) individually.

Done, thanks!

a-hilaly · 2023-08-24T21:44:49Z

pkg/model/model.go

+func CheckIfFieldExists(
+	field map[string]*Field,
+	currValue string,
+) bool {
+	_, ok := field[currValue]
+	if ok {
+		return true
+	}
+	return false
+}


Mmmmm i think this is unecessary.. it will always fall down to:

if _, ok := fieldsInParent[parent]; ok { // Check if the field is of type structure previous, res = CheckType(value, fieldsInParent) if res != true { fmt.Printf("Cannot add new field here, %s is not of type Structure", parentName) break } }

a-hilaly · 2023-08-24T21:46:20Z

pkg/model/model.go

+	return false
+}
+
+func CheckType(


Same comment here, i don't think we need this function.. or the least it should be named isShapeStructure .. and still not very convinced of moving this logic to a function

Have removed the function and added the logic to the calling function

a-hilaly · 2023-08-24T21:47:45Z

pkg/model/model.go

+type Nested struct {
+	arr    []string
+	Config *ackgenconfig.FieldConfig
+}


i know this is working. Pretty sure we can find a better way to represent this data.. (i think we are copying a lot arr)

I didn't actually get what you mean by copying arr data a lot. It is just defined once to store the names of nested fields and is then iterated over to check the nested fields type and whether they are a part of their parent field. But yeah I'm open to looking for a better way to represent the data.

a-hilaly · 2023-08-24T21:48:15Z

pkg/model/model.go

+
+		for _, toInjectNestedFields := range alltoInjectNestedFields {
+
+			addThis := ""


need a better name than addThis

Changed the name to newCustomField. Let me know if that sounds okay.

a-hilaly · 2023-08-24T21:48:40Z

pkg/model/model.go

+			var previous *Field
+			var res bool
+
+			len := len(toInjectNestedFields.arr)


len is a Go keyword, i suggest just to use len(toInjectNestedFields.arr) whenever you need to.

a-hilaly · 2023-08-24T21:49:15Z

pkg/model/model_lambda_test.go

+	assert.Contains(codeField.MemberFields, "S3SHA256")
+	assert.Contains(codeField.ShapeRef.Shape.MemberRefs, "S3SHA256")


Good testing!

a-hilaly · 2023-08-24T21:49:27Z

pkg/model/model.go

+			f := NewField(crd, fPath, memberNames, shapeRef, fConfig)
+			crd.Fields[fPath] = f
+
+			previous.MemberFields[addThis] = f


a-hilaly · 2023-08-24T21:50:14Z

pkg/model/model.go

+					// first element is a field of CRD
+					fieldsInCRD := crd.SpecFields


This could have been declared outside, it's a non necessary operation to do at every loop (same for few variables below)

There is a relation between fieldsInParent and previous variable, which changes with every loop. I can declare the fieldsInCRD outside the loop, but that is the only variable that can be defined outside the loop. To maintain the code readability I declared it the same way as fieldsInParent.

a-hilaly · 2023-08-24T21:50:47Z

pkg/model/model.go

+					if CheckIfFieldExists(fieldsInCRD, value) {
+						// Check if the field is of type structure
+						previous, res = CheckType(value, fieldsInCRD)
+						if res != true {


Suggested change

if res != true {

if !res {

This whole part is removed now

a-hilaly · 2023-09-05T18:59:45Z

/test apigatewayv2-controller-test

ack-prow · 2023-09-06T17:11:21Z

@Vandita2020: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test apigatewayv2-controller-test
/test dynamodb-controller-test
/test ec2-controller-test
/test ecr-controller-test
/test iam-controller-test
/test lambda-controller-test
/test s3-controller-test
/test unit-test

The following commands are available to trigger optional jobs:

/test s3-olm-test

Use /test all to run all jobs.

In response to this:

/test

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.

Vandita2020 · 2023-09-06T17:11:47Z

/test all

Vandita2020 · 2023-09-06T17:44:16Z

/test dynamodb-controller-test

Vandita2020 · 2023-09-06T18:54:11Z

/test dynamodb-controller-test

a-hilaly · 2023-09-14T19:46:55Z

/test all

Vandita2020 · 2023-09-17T22:56:13Z

/test iam-controller-test

valerena · 2023-09-27T17:00:40Z

pkg/model/crd.go

+// inject a field called "Password" into the "User" struct, the field path
+// would be "User.Password". The field path can be as deep as needed.
+func (crd *CRD) addCustomNestedFields(customNestedFields map[string]*ackgenconfig.FieldConfig) {
+	// Now that we collected all the nested fields, we can process, validate


What does "now" mean here? The message doesn't seem relevant in the beginning of a function.

valerena · 2023-09-27T17:01:07Z

pkg/model/crd.go

+		topLevelField := fieldParts[0]
+
+		f, ok := crd.checkSpecOrStatus(topLevelField)
+		// f, ok := crd.SpecFields[topLevelField]


Remove commented out code

valerena · 2023-09-27T17:02:00Z

pkg/model/crd.go

+	// and inject them into the Spec or Status struct.
+	for customNestedField, customNestedFieldConfig := range customNestedFields {
+		fieldParts := strings.Split(customNestedField, ".")
+		// we now than the length of fieldParts is at least 2


we know that the length

valerena · 2023-09-27T17:04:43Z

pkg/model/crd.go

+		// else {
+		// 	f, ok = crd.SpecFields[topLevelField]
+		// 	if ok && f.ShapeRef.Shape.Type != "structure" {
+		// 		// We need to panic here because the user is providing wrong configuration.
+		// 		msg := fmt.Sprintf("Expected parent field to be of type structure, but found %s", f.ShapeRef.Shape.Type)
+		// 		panic(msg)
+		// 	}
+		// }


Remove commented code

valerena · 2023-09-27T17:06:47Z

pkg/model/model.go

+		// A list of fields that should be processed after building the gathering
+		// the Spec and Status top level fields. The customNestedFields will be


the phrase "after building the gathering the Spec and Status" feels wrong. I think there's an extra word that shouldn't be there.

a-hilaly · 2023-09-27T18:09:14Z

/retest

a-hilaly

Very nice! Thanks a lot for your contribution @Vandita2020 . This will be beneficial to all the ACK controllers and it's community.
/approve
/lgtm

ack-prow · 2023-09-27T18:13:23Z

[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~~ [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 · 2023-09-27T18:31:42Z

/hold

a-hilaly · 2023-09-27T18:32:44Z

/unhold

**Issue** This PR is related to fix the issue generating after executing PR #462 **Summary** To disregard fields marked with `set: ignore` for nested fields. This PR also enhances the functionality of `set:ignore` to support the exclusion of fields from `set_sdk`. **Description** The PR fixes the issue coming after a new custom field is created and has `set.ignore` set to `true` for it. Even after setting the field to ignore, the field is getting used to set the SDK fields. In this case particularly, the new field was defined as shown below: ``` Code.S3SHA256: type: string set: - method: Create ignore: true ``` And the issue it is causing is under `sdk.go/newCreateRequestPayload` function, where it is being set, as shown below: ``` if r.ko.Spec.Code.S3SHA256 != nil { f1.SetS3SHA256(*r.ko.Spec.Code.S3SHA256) } ``` The goal is to ignore this particular line of code. And also to improve the basic functionality of `set.ignore`. The `set.ignore` functionality was till now supporting exclusion of fields only when the field was used to set for the resource. This PR checks the `set.ignore` for field and ignores the following code that creates request payload if it is set to `true`. **Limitations** Currently this PR checks the `set.ignore` field only for members of the field whose type is `structure`. **Acknowledgment** By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-prow bot requested review from jljaco and RedbackThomson August 23, 2023 23:23

Vandita2020 force-pushed the code_s3SHA256 branch from f8c6705 to 014b256 Compare August 24, 2023 17:44

a-hilaly reviewed Aug 24, 2023

View reviewed changes

Vandita2020 force-pushed the code_s3SHA256 branch from 014b256 to 88be69e Compare September 5, 2023 17:22

Vandita2020 mentioned this pull request Sep 5, 2023

Extend code generator set functionality/directive #464

Merged

Vandita2020 force-pushed the code_s3SHA256 branch from 88be69e to 285eba5 Compare September 5, 2023 23:38

Vandita2020 added 4 commits September 20, 2023 10:26

Changes to accept any nested field in the code-generator

75af4e4

Adding tests

535374d

Some changes to both test and code

963a8ac

Addressing PR review changes

eb2c009

a-hilaly mentioned this pull request Sep 25, 2023

Introduce spec.code.sha256 field to help with s3 code changes aws-controllers-k8s/lambda-controller#88

Merged

Vandita2020 force-pushed the code_s3SHA256 branch from 285eba5 to 84e7b8e Compare September 27, 2023 06:36

a-hilaly changed the title ~~Accept custom nestedField defined using 'type' in its parentField~~ Support of injecting fields in existing structures: CustomNestedFields Sep 27, 2023

Vandita2020 force-pushed the code_s3SHA256 branch from 84e7b8e to 9959155 Compare September 27, 2023 17:09

valerena reviewed Sep 27, 2023

View reviewed changes

revised changes

960ad45

Vandita2020 force-pushed the code_s3SHA256 branch from 9959155 to 960ad45 Compare September 27, 2023 18:03

a-hilaly approved these changes Sep 27, 2023

View reviewed changes

ack-prow bot assigned a-hilaly Sep 27, 2023

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2023

ack-prow bot added the approved label Sep 27, 2023

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

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

ack-prow bot merged commit 7445de3 into aws-controllers-k8s:main Sep 27, 2023
10 checks passed


		for _, toInjectNestedFields := range alltoInjectNestedFields {

		addThis := ""

		assert.Contains(codeField.MemberFields, "S3SHA256")
		assert.Contains(codeField.ShapeRef.Shape.MemberRefs, "S3SHA256")

		// first element is a field of CRD
		fieldsInCRD := crd.SpecFields

		// A list of fields that should be processed after building the gathering
		// the Spec and Status top level fields. The customNestedFields will be

Support of injecting fields in existing structures: CustomNestedFields #462

Support of injecting fields in existing structures: CustomNestedFields #462

Conversation

Vandita2020 commented Aug 23, 2023 • edited Loading

a-hilaly left a 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

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

a-hilaly commented Sep 5, 2023

ack-prow bot commented Sep 6, 2023

Vandita2020 commented Sep 6, 2023

Vandita2020 commented Sep 6, 2023

Vandita2020 commented Sep 6, 2023

a-hilaly commented Sep 14, 2023

Vandita2020 commented Sep 17, 2023

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

a-hilaly commented Sep 27, 2023

a-hilaly left a comment

Choose a reason for hiding this comment

ack-prow bot commented Sep 27, 2023

a-hilaly commented Sep 27, 2023

a-hilaly commented Sep 27, 2023

Vandita2020 commented Aug 23, 2023 •

edited

Loading