Include `Logging` as part of `Create` and `Update` flow #25

RedbackThomson · 2021-07-27T23:04:59Z

Adds custom hooks to detect changes to the Logging spec field and optionally calls the PutBucketLogging SDK method. Currently errors are not populated into terminal conditions, and updates are not supported.

Uses aws-controllers-k8s/code-generator#139 and aws-controllers-k8s/code-generator#140

RedbackThomson · 2021-07-27T23:05:16Z

/hold Please don't make me write out manual diffing methods for every one of the 24 Put* operations.

vijtrip2 · 2021-07-28T22:56:03Z

templates/hooks/bucket_sdk_post_set_output.go.tpl

+    latest := &resource{ko}
+	synced, err := rm.syncPutFields(ctx, latest)
+	if err != nil {
+		return nil, err
+	}
+	ko = synced.ko


Do the lines 2-6 have extra space in beginning?

vijtrip2 · 2021-07-28T23:01:00Z

pkg/resource/bucket/hook.go

+		return nil, err
+	}
+
+	ko := r.ko.DeepCopy()


Why is this DeepCopy needed? I noticed no changes being made in ko object.

@vijtrip2 Nick is simply copy/pasting from the normal sdk_update template... nothing really wrong with that... though techincally he should be doing if below the call to rm.sdkapi.PutBucketLogging

vijtrip2 · 2021-07-28T23:09:34Z

pkg/resource/bucket/hook.go

+	input := &svcsdk.GetBucketLoggingInput{
+		Bucket: r.ko.Spec.Name,
+	}
+	latest, err := rm.sdkapi.GetBucketLoggingWithContext(ctx, input)


latest and desired here will be of different types, correct?

It would not cause trouble to compare their primitive fields but shouldn't they both be ko.Spec.Logging? Currently latest is type from aws-sdk-go and not ACK.

Agreed. @RedbackThomson we typically use the variable name resp for the response object from the SDK call. Recommend using resp here.

jaypipes

@RedbackThomson please rebase this to remove the unrelated 0.7.1 runtime update changes...

Makefile

jaypipes

Nick, I think you can use the delta_pre_compare hook point to simplify your code a bit and remove the need for (re-)creating delta objects.

jaypipes · 2021-07-29T17:06:58Z

apis/v1alpha1/types.go

+	// all log object keys for a bucket. For more information, see PUT Bucket logging
+	// (https://docs.aws.amazon.com/AmazonS3/latest/API/RESTBucketPUTlogging.html)
+	// in the Amazon Simple Storage Service API Reference.
+	LoggingEnabled *LoggingEnabled `json:"loggingEnabled,omitempty"`


ugh. a field called "Enabled" that isn't a boolean and is a struct? :(

jaypipes · 2021-07-29T17:12:32Z

pkg/resource/bucket/hook.go

+	input := &svcsdk.GetBucketLoggingInput{
+		Bucket: r.ko.Spec.Name,
+	}
+	latest, err := rm.sdkapi.GetBucketLoggingWithContext(ctx, input)


Agreed. @RedbackThomson we typically use the variable name resp for the response object from the SDK call. Recommend using resp here.

pkg/resource/bucket/hook.go

jaypipes · 2021-07-29T17:19:43Z

pkg/resource/bucket/hook.go

+		return nil, err
+	}
+
+	ko := r.ko.DeepCopy()


@vijtrip2 Nick is simply copy/pasting from the normal sdk_update template... nothing really wrong with that... though techincally he should be doing if below the call to rm.sdkapi.PutBucketLogging

RedbackThomson · 2021-07-29T21:38:52Z

Thank you @vijtrip2 and @jaypipes for your reviews. I have updated the code-generator to remove a lot of the heavy-lifting for this - namely creating the Put* payload objects from spec. I have removed the diff method, as well, because I would instead prefer to update the fields of a resource type and then use the (already generated) diffResource code. However, that will be another pull request for supporting update operations.

RedbackThomson · 2021-07-30T04:10:06Z

Oops, I added update as well because it wasn't that much extra code. Sorry :)

jaypipes

Looking very good... couple teeny nits inline.

templates/hooks/bucket_sdk_file_end.go.tpl

RedbackThomson · 2021-07-30T19:17:57Z

/hold

I think this is ready to go, but want to wait for aws-controllers-k8s/code-generator#140 before merging.

jaypipes

👍

jaypipes · 2021-08-02T19:42:40Z

/unhold
/lgtm

ack-bot · 2021-08-02T19:42:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jaypipes, 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,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 requested review from jaypipes and vijtrip2 July 27, 2021 23:05

ack-bot added the approved label Jul 27, 2021

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

vijtrip2 reviewed Jul 28, 2021

View reviewed changes

jaypipes reviewed Jul 29, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

jaypipes suggested changes Jul 29, 2021

View reviewed changes

Nicholas Thomson added 6 commits July 29, 2021 13:39

Add logging field

825dba0

Add all other Put* fields

5f5ff9e

Add sync hook for Logging

949ea63

Support creating with TargetGrants

973dd54

Simplified create flow

ab25d7a

Generate member setters

1574d14

RedbackThomson force-pushed the put-fields branch from 7c5e02e to 1574d14 Compare July 29, 2021 20:39

Remove Makefile change

675c0c8

Support updating Logging

75be07d

RedbackThomson changed the title ~~Include Logging as part of Create flow~~ Include Logging as part of Create and Update flow Jul 30, 2021

jaypipes suggested changes Jul 30, 2021

View reviewed changes

templates/hooks/bucket_sdk_file_end.go.tpl Outdated Show resolved Hide resolved

templates/hooks/bucket_sdk_file_end.go.tpl Outdated Show resolved Hide resolved

templates/hooks/bucket_sdk_file_end.go.tpl Outdated Show resolved Hide resolved

Rename createBucketLoggingStatus to newBucketLoggingStatus

8ba9177

Nicholas Thomson added 4 commits July 30, 2021 12:23

Add support for describing put fields

1b65a49

Remove local-run-controller

e29b99e

Fix template indentation

d1e8b5b

Update code-gen method names

0a9980a

jaypipes approved these changes Aug 2, 2021

View reviewed changes

ack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2021

ack-bot assigned jaypipes Aug 2, 2021

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

ack-bot merged commit a544e43 into aws-controllers-k8s:main Aug 2, 2021

RedbackThomson deleted the put-fields branch August 2, 2021 20:10

Include Logging as part of Create and Update flow #25

Include Logging as part of Create and Update flow #25

Uh oh!

Conversation

RedbackThomson commented Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RedbackThomson commented Jul 27, 2021

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!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RedbackThomson commented Jul 29, 2021

Uh oh!

RedbackThomson commented Jul 30, 2021

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RedbackThomson commented Jul 30, 2021

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

jaypipes commented Aug 2, 2021

Uh oh!

ack-bot commented Aug 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Include `Logging` as part of `Create` and `Update` flow #25

Include `Logging` as part of `Create` and `Update` flow #25

RedbackThomson commented Jul 27, 2021 •

edited

Loading