Create custom list and map fields #222

RedbackThomson · 2021-10-14T01:39:09Z

Issue #, if available: aws-controllers-k8s/community#881

Description of changes:
Supports creating custom list and map fields where the member shapes of those fields already exist in the SDK.
Rather than supplying a from in the field config, you can now supply a custom_field property which requires
either list_of or map_of with the name of an API shape as the value.

An example of generating a list (for an S3 Bucket):

resources:
  Bucket:
    fields:
      AnalyticsConfigurations:
        custom_field:
          list_of: AnalyticsConfiguration

Produces the following:

AnalyticsConfigurations []*AnalyticsConfiguration `json:"analyticsConfigurations,omitempty"`

An example of generating a map (for an S3 Bucket):

resources:
  Bucket:
    fields:
      AnalyticsConfigurations:
        custom_field:
          map_of: AnalyticsConfiguration

Produces the following:

AnalyticsConfigurations map[string]*AnalyticsConfiguration `json:"analyticsConfigurations,omitempty"`

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

ack-bot · 2021-10-14T01:39:11Z

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

RedbackThomson · 2021-10-14T01:42:39Z

Waiting for rebase on top of #221

brycahta

The refactor from model to operations makes things a lot clearer 👍

cmd/ack-generate/command/common.go

brycahta · 2021-10-18T14:53:57Z

pkg/generate/ack/controller.go

@@ -22,6 +22,7 @@ import (
 	ackgenconfig "github.com/aws-controllers-k8s/code-generator/pkg/generate/config"
 	"github.com/aws-controllers-k8s/code-generator/pkg/generate/templateset"
 	ackmodel "github.com/aws-controllers-k8s/code-generator/pkg/model"
+	ackoperations "github.com/aws-controllers-k8s/code-generator/pkg/operations"


why not use this import alias everywhere? Such as in check.go below

The import aliasing in ACK seems to be a bit random. Some places use ackmodel others import model without an alias. I have just stuck to whatever the file has as convention. I don't think ackmodel is necessary, because we are clearly in the ack project, so model should be sufficient.

pkg/generate/config/field.go

pkg/sdk/custom_shapes.go

pkg/sdk/helper.go

brycahta · 2021-10-18T18:43:33Z

pkg/sdk/helper.go

+			typeRenames:    nil,
+		}
+
+		h.InjectCustomShapes(sdkapi)


From the current implementation of Helper, I don't think it makes sense for one of its methods to be able to change/edit sdkapi.

Is there any reason this couldn't be a setter? ex: func (a *SDKAPI) SetCustomShapes(cfg ackgenconfig.Config)

My aim for some of the refactoring work was that SDKAPI is an artifact produced by the sdk.Helper struct. "Injecting" it here, is just one stage of creating that artifact - before it is eventually passed to the model struct initializer.

Because of tight code coupling, SDKAPI has to stay in the model package, but ideally it would be owned in the sdk package.

pkg/sdk/helper.go

pkg/sdk/custom_shapes.go

brycahta · 2021-10-18T18:54:24Z

pkg/sdk/helper.go

 	h.apiVersion = apiVersion
 }

 // API returns the aws-sdk-go API model for a supplied service model name.
-func (h *SDKHelper) API(serviceModelName string) (*SDKAPI, error) {
+func (h *Helper) API(serviceModelName string) (*SDKAPI, error) {


I know you didn't change any logic here, but why have the helper do this instead of a constructor for SDKAPI requiring serviceModelName?

I don't have any history for this method - they predate my introduction to the project. My best guess is that we don't care about the service name anywhere else in the Helper, but I think over time we have expanded it and now we have an awkward waterfall of passing it between methods.

RedbackThomson · 2021-10-21T20:06:13Z

/test s3-controller-test

brycahta

Is the only way to use this custom field via Hooks in service-controller code?

brycahta · 2021-10-21T19:33:20Z

pkg/model/types_test.go

 	"github.com/aws-controllers-k8s/code-generator/pkg/model"
+	"github.com/stretchr/testify/assert"


nit: import order

Ugh damn linter, again.

brycahta · 2021-10-21T20:20:01Z

pkg/model/model.go

+					)
+					panic(msg)
+				}
+			} else if fieldConfig.CustomField != nil {


It seems like the custom field is being added to both Spec and Status-- is that intended? Am I misreading?

The field can be added to either the Spec or the Status, depending on the value of is_read_only. Otherwise the implementations don't discriminate.

RedbackThomson · 2021-10-21T20:27:33Z

Is the only way to use this custom field via Hooks in service-controller code?

At the moment, yes. Considering the type doesn't exist in the SDK, the generator won't really know how to use it - it's not linked to any of the operations and therefore not linked to any SDK methods. This could be smarter in the future, but not in this revision.

brycahta

/lgtm

ack-bot · 2021-10-21T20:29:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brycahta, RedbackThomson

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~~ [RedbackThomson]

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

Implements aws-controllers-k8s/community#914 Description of changes: Uses the new custom fields code-generator PR (aws-controllers-k8s/code-generator#222) to create lists of configuration types. At runtime each of these configurations is put individually (mapped using their ID field) onto the bucket. For each reconciliation, we describe the configurations using a `List*` operation and determine the delta for each configuration using a generated `compare*` method in the `sdk.go`. The configuration elements are individually created, deleted, updated or no-op'd depending on the diff. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

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 Oct 14, 2021

ack-bot requested review from jaypipes and vijtrip2 October 14, 2021 01:39

ack-bot added the approved label Oct 14, 2021

brycahta suggested changes Oct 18, 2021

View reviewed changes

Add create custom map-of and list-of types

d501b02

RedbackThomson force-pushed the custom-fields branch from 42429aa to d501b02 Compare October 21, 2021 18:35

Nicholas Thomson added 3 commits October 21, 2021 11:49

Update code comments and clean signatures

97f6a9e

Move config getters onto config

c36468e

Add unit tests for custom shapes

4be099a

RedbackThomson marked this pull request as ready for review October 21, 2021 19:44

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 Oct 21, 2021

RedbackThomson requested a review from brycahta October 21, 2021 19:44

brycahta reviewed Oct 21, 2021

View reviewed changes

brycahta approved these changes Oct 21, 2021

View reviewed changes

ack-bot assigned brycahta Oct 21, 2021

ack-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2021

ack-bot merged commit af5f01e into aws-controllers-k8s:main Oct 21, 2021

RedbackThomson deleted the custom-fields branch October 21, 2021 20:30

This was referenced Oct 21, 2021

Is it possible to inject new fields during code generation? aws-controllers-k8s/community#881

Closed

Support Put* fields for slices of configuration types aws-controllers-k8s/s3-controller#57

Merged

RedbackThomson mentioned this pull request May 10, 2022

Creating new status fields for RDS DBInstance CRD aws-controllers-k8s/community#1289

Closed

		"github.com/aws-controllers-k8s/code-generator/pkg/model"
		"github.com/stretchr/testify/assert"

Create custom list and map fields #222

Create custom list and map fields #222

Uh oh!

Conversation

RedbackThomson commented Oct 14, 2021

Uh oh!

ack-bot commented Oct 14, 2021

Uh oh!

RedbackThomson commented Oct 14, 2021

Uh oh!

brycahta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RedbackThomson commented Oct 21, 2021

Uh oh!

brycahta 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!

RedbackThomson commented Oct 21, 2021

Uh oh!

brycahta left a comment

Choose a reason for hiding this comment

Uh oh!

ack-bot commented Oct 21, 2021

Uh oh!

Uh oh!