-
-
Notifications
You must be signed in to change notification settings - Fork 678
[federation] Add make_leave & send_leave #535
Conversation
// MakeLeave implements the /make_leave API | ||
func MakeLeave( | ||
ctx context.Context, | ||
httpReq *http.Request, |
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.
Surely you can just pass httpReq and then take the Context from that?
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.
yes, didn't notice this redundancy.
I wonder if we can change LogThenError
to take just the httpReq.Context()
instead of the whole request.
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.
IIRC this split was made by intention...
// SendLeave implements the /send_leave API | ||
func SendLeave( | ||
ctx context.Context, | ||
httpReq *http.Request, |
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.
Same here.
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.
We probably also want to double check that the membership is set to leave, otherwise LGTM
Signed-off-by: Anant Prakash <anantprakashjsr@gmail.com>
Signed-off-by: Anant Prakash <anantprakashjsr@gmail.com>
Oh, oops, you misunderstood what I meant, sorry. I meant that when we check that the various fields, like that |
Thanks! :D |
* [federation] Add make_leave & send_leave * Remove redundant parameters * Check membership is set to leave Signed-off-by: Anant Prakash <anantprakashjsr@gmail.com>
Mostly the same as make_join & send_join.