Initial version of CEL resource expressions. #928
Conversation
| @@ -63,10 +63,24 @@ type ReceiverSpec struct { | |||
| // +optional | |||
| Events []string `json:"events,omitempty"` | |||
|
|
|||
| // TODO: Validate one or other (or both?) | |||
|
|
|||
There was a problem hiding this comment.
This will need to be addressed, because you could have .spec.resourceExpressions or .spec.resources (or both).
bigkevmcd
force-pushed
the
cel-notification-resources
branch
3 times, most recently
from
786ff7e
to
5da3f91
Compare
There was a problem hiding this comment.
Thanks for the work here @bigkevmcd, but I think we need to create an RFC or at least an issue to discuss this feature.
| // Only decodes the body for the expression if the body is JSON. | ||
| // Technically you could generate several resources without any body. | ||
| if isJSONContent(r) { | ||
| if err := json.NewDecoder(r.Body).Decode(&body); err != nil { |
There was a problem hiding this comment.
This needs to be reworked, the request body was already received at this point, in the line if err := s.validate(ctx, receiver, r); err != nil { from (s *ReceiverServer) handlePayload(). Reading it again won't be possible. To fix this you will need something like a https://pkg.go.dev/io#TeeReader with a https://pkg.go.dev/bytes#Buffer.
There was a problem hiding this comment.
I missed this in the code above, I'll fix this.
There was a problem hiding this comment.
Another option is simply reading the body into a bytes.Buffer and passing it to all the parts of the code that need to consume it.
Looking again at the original issue, maybe we could use that issue to discuss this feature, it looks like in this PR you're specifically trying to address this part:
|
This adds support for dynamic generation of resources for receivers using CEL and parsing the body in the content request. Signed-off-by: Kevin McDermott <[email protected]>
bigkevmcd
force-pushed
the
cel-notification-resources
branch
from
b6d22ba
to
8cfcfb6
Compare
|
To fix the e2e GitHub Action failing you can run |
Signed-off-by: Kevin McDermott <[email protected]>
bigkevmcd
force-pushed
the
cel-notification-resources
branch
from
de123a3
to
4e2654f
Compare
|
Now there's a diff on |
That's ok, it was a quick branch to explore what's possible, there's no compelling need to land this, and, it's taking on support for CEL, which is not "free". Worst case, it gets closed as "won't fix" 😄 and informs a different solution, fine by me.
Yes, this is basically a way to dynamically generate the set of resources to notify in a receiver. The new field |
bigkevmcd
force-pushed
the
cel-notification-resources
branch
from
1d7d611
to
e25d8f3
Compare
|
This solution sounds very elegant to me @bigkevmcd, I'd love to see it in. It would be nice if you could attend some of our dev meetings to discuss this feature, they are public and can be found here: https://fluxcd.io/community/#meetings (The |
|
I'll see what I can do to be there, time permitting. You'll see that the specific request in here #491 (comment) is tested in the code. |
bigkevmcd
force-pushed
the
cel-notification-resources
branch
from
e25d8f3
to
1d7d611
Compare
This is a very rough cut (needs additional error checking).
But the basic implementation works.
This is the code I referenced here #491 (comment)
I will harden it up, checking that it's receiving a
jsonbody and improve the error handling, this was a quick bit of code to see if it was possible.