Skip to content

Add FileTransferRequestEvent#24558

Merged
avatus merged 1 commit intomasterfrom
michaelmyers/add_file_transfer_request_event
Apr 13, 2023
Merged

Add FileTransferRequestEvent#24558
avatus merged 1 commit intomasterfrom
michaelmyers/add_file_transfer_request_event

Conversation

@avatus
Copy link
Copy Markdown
Contributor

@avatus avatus commented Apr 13, 2023

Supports: #23546

This PR only adds a single event, but the proto changes were quite substantial so I'm breaking this chunk out to clean up the next, larger PRs

@github-actions github-actions Bot requested review from atburke and smallinsky April 13, 2023 18:28
@avatus avatus force-pushed the michaelmyers/add_file_transfer_request_event branch from ba33ff2 to 0b917bb Compare April 13, 2023 18:28
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
string RequestID = 3 [(gogoproto.jsontag) = "requestId"];
string RequestID = 3 [(gogoproto.jsontag) = "requestID"];

Comment on lines 632 to 633
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch to enum type ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet, idk why i didn't think of this. great idea!

Comment on lines 611 to 614
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want to include the user metadata?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can but I don't see it being necessary at this point. The process doesn't really "care" who did it, as the approvers are stored in the request, as well as broadcasted (in the future PR) to the shared session on each action. This is mostly just an event to notify an updated happened to the state of the event.

Copy link
Copy Markdown
Contributor

@smallinsky smallinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't caught up the full context of the RFD but per this protobuf change LGTM.

Comment on lines 632 to 633
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could you updated the comment and describe the the context of direction

Comment on lines 639 to 643
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing go docs.

@avatus avatus force-pushed the michaelmyers/add_file_transfer_request_event branch 2 times, most recently from c93a185 to 64ebb29 Compare April 13, 2023 18:59
@avatus avatus force-pushed the michaelmyers/add_file_transfer_request_event branch from 64ebb29 to d1dc225 Compare April 13, 2023 19:09
@avatus avatus enabled auto-merge April 13, 2023 19:19
@avatus avatus added this pull request to the merge queue Apr 13, 2023
Merged via the queue into master with commit e512827 Apr 13, 2023
@avatus avatus deleted the michaelmyers/add_file_transfer_request_event branch April 13, 2023 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants