Change 'proto' folder name to 'msg' folder and add riskFactor/status to msg.Order - [merged] #849

Closed ashleyvega closed 4 years ago

In GitLab by @cdm on Jul 19, 2018, 17:51

Merges feature/rename-proto-folder-and-add-status-order -> develop

This is a maintenance change to support a) new core fields on Order b) package in go did not match package in protobuf which causes issues when autogenerating from other plugins e.g. graphql

Overview, large refactor of proto folder name to msg, the imports to lots of files have changed, added riskFactor and status to msg.Order. Please merge EARLY to avoid merge hell and disappointment ;)

In GitLab by @cdm on Jul 19, 2018, 17:53

@makspachucki or @dmgarland please can you review and merge at your earliest convenience :juggler:

In GitLab by @cdm on Jul 20, 2018, 13:23

added 2 commits

4d89b647 - 1 commit from branch develop
32e6f310 - Large refactor of proto folder name to msg, the imports to lots of files have…

Compare with previous version

In GitLab by @makspachucki on Jul 20, 2018, 13:23

approved this merge request

In GitLab by @dmgarland on Jul 20, 2018, 13:26

This is precisely the discussion I wanted to have before as to what the folder should be called. Rather than put in a merge like this unilaterally, I thought it would be good to decide first.

In GitLab by @dmgarland on Jul 20, 2018, 13:26

I don't recall this being mentioned in standup either

In GitLab by @cdm on Jul 20, 2018, 13:28

I know we need to review your grpc branch, BUT... before this gets political...

This merge request was made last night, and the protobuf package has ALWAYS been msg. The folder was simply named incorrectly and causes issues with the tools so this fixes this BUG.

The other changes to structure are to support my work with Cancellation

In GitLab by @cdm on Jul 20, 2018, 13:29

(I did add riskFactor to help your merging)

In GitLab by @cdm on Jul 20, 2018, 13:32

enabled an automatic merge when the pipeline for 32e6f310f93e3bbc1dc7fe9b37f92bb93a060e8a succeeds

In GitLab by @dmgarland on Jul 20, 2018, 13:32

canceled the automatic merge

In GitLab by @dmgarland on Jul 20, 2018, 13:33

I agree msg is definitely the better package name. It's just instead of sitting down and deciding where this stuff goes and building on work I've done we've just gone through and changed everything. I mean, if I just forced pushed a bunch of stuff to develop without discussion I doubt anyone would appreciate it very much. I've also asked for this conversation to happen for nearly a week and keep getting put off.

Why don't we just jump on a call and figure out an approach that suits everyone?

In GitLab by @cdm on Jul 20, 2018, 13:35

The grpc package name has ALWAYS been msg, this commit fixes the naming of the folder in the project to suit.

Is your current task finished @dmgarland we agreed you do that so we could merge it?

In GitLab by @cdm on Jul 20, 2018, 13:36

I'm happy to review later today as you suggested, and I'd like maks on the call too. I'm pretty fed up of this groundhog day conversation so lets put it to bed.

In GitLab by @cdm on Jul 20, 2018, 15:49

Outcome: Maks Dan and I have had a call to go over the GRPC branch and MR. We've discussed that we need to improve communication and agreed that we'll take the following through as technical dept tasks:

Improvements to swagger docs/definitions to be added as a separate branch/MR. Goal: improved docs.
Separate out the GRPC services into trade, order, market, etc at a suitable point (with discussion, as requested). Goal: clearer separation of services.
Dan and Maks or Chris to discuss and review the api services layer re testability, separation of concerns etc. Goal: review and knowledge transfer.

If I have missed any specific actionable tasks, please add below. These will be added to our tech debt 'wall' in Eddra for now

In GitLab by @cdm on Jul 20, 2018, 16:02

merged

vegaprotocol / vega

Change 'proto' folder name to 'msg' folder and add riskFactor/status to msg.Order - [merged] #849