Add `Pipes` Resource by embano1 · Pull Request #1 · aws-controllers-k8s/pipes-controller

embano1 · 2023-02-27T12:54:08Z

Description of changes: Add Pipes Resource

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

apis/v1alpha1/types.go

go.mod

apis/v1alpha1/types.go

OWNERS_ALIASES

pkg/resource/pipe/hooks.go

OWNERS_ALIASES

embano1 · 2023-03-03T15:00:17Z

I'll split the two commits into two PRs as discussed with @a-hilaly

Signed-off-by: Michael Gasch <15986659+embano1@users.noreply.github.com>

embano1 · 2023-03-10T16:14:42Z

pkg/resource/pipe/hooks.go

+
+// hasNilDifference returns true if the supplied subjects' nilness is
+// different
+func hasNilDifference(a, b interface{}) bool {


99% copy and paste from runtime and code-gen because we need to treat empty struct responses as nil :/
Changing this in the runtime would very likely lead to breaking changes.

embano1 · 2023-03-10T16:19:12Z

apis/v1alpha1/generator.yaml

+  client_interface: PipesAPI
+resources:
+  Pipe:
+    fields:


Let me know if we need custom compare for tags as per API def they're a map type (which I guess you'd consider non-deterministic) -> Tags.

Just added one :)

Signed-off-by: Michael Gasch <15986659+embano1@users.noreply.github.com>

jaypipes

I'm almost there on this one. I do, however, think it would be best to rename Spec.DesiredState to just Spec.State and ignore the Status.CurrentState field path (see note inline...

generator.yaml

jaypipes · 2023-03-14T14:04:57Z

go.mod

 require (
 	github.com/aws-controllers-k8s/runtime v0.24.1
-	github.com/aws/aws-sdk-go v1.44.218
+	github.com/aws/aws-sdk-go v1.44.209


Any reason we're going back 9 releases?

jaypipes · 2023-03-14T14:05:27Z

helm/Chart.yaml

  - name: ACK Admins
    url: https://github.com/orgs/aws-controllers-k8s/teams/ack-admin
-  - name:  Admins
+  - name: Pipes Admins


You can just use my full name here. ;)

Sure you can handle that :-p ?

jaypipes · 2023-03-14T14:14:25Z

pkg/resource/pipe/hooks.go

+	// hack (by the great @a-hilaly): forces a requeue in update if currentState != desiredState to reconcile
+	// and update status fields in Kubernetes resource e.g., in case of UPDATE_FAILED
+	if *bDesiredState != *b.ko.Status.CurrentState {
+		// setting Spec. because Status. is not considered in delta logic
+		delta.Add("Spec.CurrentState", *bDesiredState, *b.ko.Status.CurrentState)
+	}


Instead of doing this, you could rename the DesiredState field to just State, then ignore the CreatePipeOutput.CurrentState field path and in the sdk_read_one_post_set_output code hook, simply do:

if resp.CurrentState != nil { ko.Spec.State = resp.CurrentState }

then the standard comparison logic would be just fine.

A better hack by the great EventBridge "Jay" Pipes

The only concern I have here is from a debugging pov since an operator could be confused a) by the field name and b) why this field name has a value which is not allowed as per API:

DesiredState
The state the pipe should be in.

Type: String

Valid Values: RUNNING | STOPPED

Required: No

@embano1 @a-hilaly OK, I didn't realize the value set of these fields was different. I see now that this is more of a way for EventBridge to avoid having a StopPipe API call that only changes the desired state of the pipe. Alright, then, good to go with this :)

pkg/resource/pipe/hooks.go

a-hilaly · 2023-03-14T23:52:21Z

@jaypipes took me some time to think about this but I'm a little bit confused about why would we remove the CurrentState field. it's very similar to what rds, dyamodb, ec2... etc have. It makes sense to keep the field so it's viewable via kubectl describe and we could also inject it in the kubectl get response. Thoughts?

a-hilaly

Alright lads let's ship this one!
Dankeshoen @embano1 and @jaypipes!
/lgtm

embano1 · 2023-03-15T15:41:44Z

Awesome, thx a ton for your help throughout these EDA controllers :)

embano1 · 2023-03-15T15:51:04Z

pkg/resource/pipe/hooks.go

+//  _____ _     _____ _      _____  ____  ____  _  ____  _____ _____
+// /  __// \ |\/  __// \  /|/__ __\/  _ \/  __\/ \/  _ \/  __//  __/
+// |  \  | | //|  \  | |\ ||  / \  | | //|  \/|| || | \|| |  _|  \
+// |  /_ | \// |  /_ | | \||  | |  | |_\\|    /| || |_/|| |_//|  /_
+// \____\\__/  \____\\_/  \|  \_/  \____/\_/\_\\_/\____/\____\\____\
+//
+// _  _    _  ____ ___  __  _   ____  _  ____  _____ ____
+// \||/   / |/  _ \\  \//\||/  /  __\/ \/  __\/  __// ___\
+//        | || / \| \  /       |  \/|| ||  \/||  \  |    \
+//     /\_| || |-|| / /        |  __/| ||  __/|  /_ \___ |
+//     \____/\_/ \|/_/         \_/   \_/\_/   \____\\____/
+


🤣 Your friendly 24/7 support

Signed-off-by: Amine Hilaly <hilalyamine@gmail.com>

a-hilaly · 2023-03-15T17:09:55Z

/lgtm

ack-prow · 2023-03-15T17:10:02Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

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

embano1 changed the title ~~Pipes~~ Add Pipes Resource Feb 27, 2023

ack-prow bot added the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Feb 27, 2023

RedbackThomson reviewed Feb 28, 2023

View reviewed changes

apis/v1alpha1/types.go Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

embano1 commented Mar 2, 2023

View reviewed changes

apis/v1alpha1/types.go Show resolved Hide resolved

embano1 force-pushed the pipes branch 2 times, most recently from 6475e0a to 86df0fa Compare March 2, 2023 14:37

RedbackThomson reviewed Mar 2, 2023

View reviewed changes

OWNERS_ALIASES Outdated Show resolved Hide resolved

pkg/resource/pipe/hooks.go Outdated Show resolved Hide resolved

pkg/resource/pipe/hooks.go Outdated Show resolved Hide resolved

embano1 force-pushed the pipes branch from 86df0fa to 9e7d326 Compare March 3, 2023 08:53

embano1 commented Mar 3, 2023

View reviewed changes

OWNERS_ALIASES Outdated Show resolved Hide resolved

embano1 force-pushed the pipes branch from 9e7d326 to 9c90fd6 Compare March 3, 2023 10:48

ack-prow bot removed the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Mar 3, 2023

embano1 force-pushed the pipes branch from 9c90fd6 to c0c94e0 Compare March 9, 2023 15:38

Controller Bootstrap

ecd91d6

Signed-off-by: Michael Gasch <15986659+embano1@users.noreply.github.com>

embano1 force-pushed the pipes branch 2 times, most recently from c9f710e to 9c34d77 Compare March 10, 2023 16:02

embano1 commented Mar 10, 2023

View reviewed changes

Add Pipes Resource

858d9fd

Signed-off-by: Michael Gasch <15986659+embano1@users.noreply.github.com>

embano1 force-pushed the pipes branch from 9c34d77 to 858d9fd Compare March 10, 2023 16:28

a-hilaly force-pushed the pipes branch 10 times, most recently from 7bceb14 to 8a1fcaa Compare March 14, 2023 00:58

a-hilaly force-pushed the pipes branch 4 times, most recently from d4f4f81 to ca538b3 Compare March 14, 2023 02:47

a-hilaly mentioned this pull request Mar 14, 2023

Release artifacts for release v0.0.1 #3

Merged

a-hilaly force-pushed the pipes branch 5 times, most recently from 980a59a to 2fa02eb Compare March 14, 2023 13:52

jaypipes reviewed Mar 14, 2023

View reviewed changes

a-hilaly force-pushed the pipes branch from 2fa02eb to 2934f8e Compare March 14, 2023 23:48

jaypipes approved these changes Mar 15, 2023

View reviewed changes

a-hilaly force-pushed the pipes branch 2 times, most recently from 88163f5 to b27016b Compare March 15, 2023 15:32

a-hilaly approved these changes Mar 15, 2023

View reviewed changes

ack-prow bot assigned a-hilaly Mar 15, 2023

ack-prow bot added lgtm Indicates that a PR is ready to be merged. approved labels Mar 15, 2023

embano1 commented Mar 15, 2023

View reviewed changes

Add e2e tests for pipe resource

2121324

Signed-off-by: Amine Hilaly <hilalyamine@gmail.com>

a-hilaly force-pushed the pipes branch from b27016b to 2121324 Compare March 15, 2023 16:58

ack-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 15, 2023

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

ack-prow bot merged commit 299f342 into aws-controllers-k8s:main Mar 15, 2023

embano1 deleted the pipes branch March 22, 2023 15:55

Comments

Conversation

embano1 commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

embano1 commented Mar 3, 2023

Uh oh!

embano1 Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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 Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

embano1 Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

a-hilaly commented Mar 14, 2023

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

embano1 commented Mar 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly commented Mar 15, 2023

Uh oh!

ack-prow bot commented Mar 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

embano1 commented Feb 27, 2023 •

edited

Loading

embano1 Mar 10, 2023 •

edited

Loading

a-hilaly Mar 14, 2023 •

edited

Loading

embano1 Mar 14, 2023 •

edited

Loading