-
Notifications
You must be signed in to change notification settings - Fork 600
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat:added format flag #8080
feat:added format flag #8080
Conversation
Signed-off-by: EraKin575 <[email protected]>
Welcome @EraKin575! It looks like this is your first PR to knative/eventing 🎉 |
Hi @EraKin575. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
Signed-off-by: EraKin575 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @EraKin575, left a few comments
pkg/apis/duck/v1/delivery_types.go
Outdated
@@ -81,6 +81,11 @@ type DeliverySpec struct { | |||
// | |||
// +optional | |||
RetryAfterMax *string `json:"retryAfterMax,omitempty"` | |||
|
|||
//Format supports more destinations of cloudevents (which may require a specific event format) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would change the description here to be more specific
Something like
Format specifies the event format sent to subscriber and dead letter sink.
Valid values are:
- `json` <add link_to_cloudevents_spec_json_strucutred_format>
- `binary` <add link_to_cloudevents_spec binary format for HTTP>
- `ingress` <add description>
pkg/apis/duck/v1/delivery_types.go
Outdated
@@ -123,6 +128,17 @@ func (ds *DeliverySpec) Validate(ctx context.Context) *apis.FieldError { | |||
} | |||
} | |||
|
|||
if ds.Format != nil { | |||
validFormats := map[string]bool{ | |||
"structured": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Cali0707 I was thinking now, since "structured" is not an event format but rather "JSON" is an event format, should we use JSON
, so that if we would need to support "XML" or "protobuf" we could?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point- let's use JSON
instead then. @EraKin575 can you make the necessary changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will
pkg/apis/duck/v1/delivery_types.go
Outdated
"binary": true, | ||
"ingress": true, | ||
} | ||
if !validFormats[*ds.Format] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do a case insensitive comparison !validFormats[strings.ToLower(*ds.Format)]
"ingress": true, | ||
} | ||
if !validFormats[*ds.Format] { | ||
errs = errs.Also(apis.ErrInvalidValue(*ds.Format, "format")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add the details of the error with the list of valid values (3rd parameter to ErrInvalidValue)
apis.ErrInvalidValue(*ds.Format, "format", <detailed error>)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! I will make the changes
pkg/apis/duck/v1/delivery_types.go
Outdated
"binary": true, | ||
"ingress": true, | ||
} | ||
if !validFormats[*ds.Format] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This panics if ds.Format
is not in the map, we need to do something like
_, ok := validFormats[*ds.Format]; !ok {
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a few test cases for the validation of Format
in
func TestDeliverySpecValidation(t *testing.T) { |
@EraKin575 you will also need to run |
Signed-off-by: EraKin575 <[email protected]>
Signed-off-by: EraKin575 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
/ok-to-test
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Cali0707, EraKin575 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:
Approvers can indicate their approval by writing |
merged my first PR at knative! |
Fixes #8073
added format flag to deliverytype struct to support more destination of cloudevents
Proposed Changes
Pre-review Checklist
Release Note
Docs