-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
event sink crud operation api #9155
Conversation
2b87e74
to
1f3ccc9
Compare
@@ -0,0 +1,155 @@ | |||
package structs |
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.
Good call on moving these out to their own file 👍
942fc69
to
10aff7e
Compare
nomad/structs/structs.go
Outdated
@@ -97,6 +97,8 @@ const ( | |||
ScalingEventRegisterRequestType | |||
CSIVolumeClaimBatchRequestType | |||
CSIPluginDeleteRequestType | |||
EventSinkUpsertRequestType | |||
EventSinkDeleteRequestType |
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.
Oof I caused a merge conflict with this and raftutil msgtypes in #9135
Sorry!
state store methods and restore upsert sink test get sink delete sink event sink list and tests go generate new msg types validate sink on upsert
10aff7e
to
dcf51a8
Compare
@@ -17,6 +17,103 @@ import ( | |||
"golang.org/x/sync/errgroup" | |||
) | |||
|
|||
func (s *HTTPServer) EventSinksRequest(resp http.ResponseWriter, req *http.Request) (interface{}, error) { | |||
if req.Method != http.MethodGet { | |||
return nil, CodedError(405, ErrInvalidMethod) |
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.
nit: there's https://golang.org/pkg/net/http/#pkg-constants we can use instead of raw numbers, e.g. http.StatusMethodNotAllowed
return nil, CodedError(405, ErrInvalidMethod) | ||
} | ||
|
||
args := structs.EventSinkListRequest{} |
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.
hyper nit: using :=
here, but var
below for instantiating default value structs
@@ -22,6 +23,95 @@ type testEvent struct { | |||
ID string | |||
} | |||
|
|||
func TestHTTP_EventSinkList(t *testing.T) { | |||
t.Parallel() |
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 test is parallel but the others are not, can they all be?
|
||
// Ensure we never set the index to zero, otherwise a blocking query cannot be used. | ||
// We floor the index at one, since realistically the first write must have a higher index. | ||
if index == 0 { |
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.
could be even more sure with index <= 0
|
||
func (j *EventJson) Copy() *EventJson { | ||
n := new(EventJson) | ||
*n = *j |
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 doesn't seem to do anything, the only field is Data
and we need to overwrite it with a deep copy anyway
mErr.Errors = append(mErr.Errors, fmt.Errorf("Sink ID contains a space")) | ||
} else if strings.Contains(e.ID, "\000") { | ||
mErr.Errors = append(mErr.Errors, fmt.Errorf("Sink ID contains a null character")) | ||
} |
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.
Do we need to be this lenient? Seems like \t
would slip though this for example. Could we not use a relatively plain regex like we do for namespace?
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 was following Job, namespace seems too constrictive 🤔
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.
\t doesn't seem great either though, I don't see why we'd want to not allow dash or underscore, not sure if there's precedent for anything less rigorous than namespaces
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
CRUD operations and plumbing 🛠️ for managing event stream sinks