Skip to content
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

Improve comments for RPC funcs #1287

Merged
merged 7 commits into from
Nov 23, 2020
Merged

Improve comments for RPC funcs #1287

merged 7 commits into from
Nov 23, 2020

Conversation

hsorellana
Copy link
Contributor

What this PR does / Why we need it:
This PR adds some modifications to the comments of the Backfill funcs inside the FrontendService since they weren't so easy to read and understand its usage.

@google-cla google-cla bot added the cla: yes label Nov 17, 2020
Copy link
Collaborator

@aLekSer aLekSer left a comment

Choose a reason for hiding this comment

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

Changes in proto file is good. A bit worried of why swagger.json so significantly changed. Can you test this with kind and make proxy-ui to see if it looks good on http://localhost:51500/:

make create-kind-cluster
make get-kind-kubeconfig
make push-images
make install-chart

@@ -111,8 +111,7 @@ message GetBackfillRequest {
// BETA FEATURE WARNING: This Request message is not finalized and still subject
// to possible change or removal.
message UpdateBackfillRequest {
// A Backfill object with SearchFields defined.
Backfill backfill_ticket = 1;
Backfill backfill = 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this more as it is consistent with CreateBackfillRequest. 👍

"description": "BETA FEATURE WARNING: This call and the associated Request and Response\nmessages are not finalized and still subject to possible change or removal.",
"operationId": "FrontendService_DeleteBackfill",
Copy link
Collaborator

@aLekSer aLekSer Nov 17, 2020

Choose a reason for hiding this comment

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

Not sure why there are so many changes to swagger.json files. Do you use a different version of it?

@hsorellana
Copy link
Contributor Author

i used an online swagger editor to verify how the output json file looks like. And it looks like
Screen Shot 2020-11-17 at 18 55 37

@aLekSer
Copy link
Collaborator

aLekSer commented Nov 18, 2020

Tried to regenerate swagger.json files, they are the same as in this PR.

@syntxerror syntxerror modified the milestones: v1.1.0, v1.2.0 Nov 18, 2020

// Side effects: Any tickets waiting for this backfill will be returned to the active pool, no longer pending.

// UpdateBackfill updates a Backfill object with an ID set and SearchFields defined.

Choose a reason for hiding this comment

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

extensions will be updated to. Perhaps something like "UpdateBackfill updates search_fields and extensions for the backfill with the provided id"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied!

pkg/pb/evaluator.pb.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@aLekSer aLekSer left a comment

Choose a reason for hiding this comment

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

Looks nice.

@Laremere Laremere merged commit aa4398e into googleforgames:master Nov 23, 2020
@hsorellana hsorellana deleted the fix-comments branch November 23, 2020 19:39
@syntxerror syntxerror added area/developer-experience This issue impacts developer experience when building a Matchmaker using Open Match area/refinement labels Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/developer-experience This issue impacts developer experience when building a Matchmaker using Open Match area/refinement cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants