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

write query parameters to swagger definition #297

Merged
merged 3 commits into from
Jan 25, 2017

Conversation

t-yuki
Copy link
Contributor

@t-yuki t-yuki commented Jan 12, 2017

It takes over from #199

"name": "map_value",
"in": "query",
"required": false,
"type": "object"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

map values is hard to process in query parameter. (some WAF handles it in map_value[KEY]=VALUE)
should we ignore it initially?

@t-yuki t-yuki force-pushed the swagger-query-parameters branch from 8a20710 to 1380e7f Compare January 13, 2017 19:08
@t-yuki t-yuki changed the title [WIP] write query parameters to swagger definition write query parameters to swagger definition Jan 13, 2017
@t-yuki t-yuki force-pushed the swagger-query-parameters branch from 1380e7f to 4d81b7a Compare January 14, 2017 06:47
"ZERO",
"ONE"
],
"default": "ZERO"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the default be ["ZERO"]? or maybe []?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. I unset default field.
Note that, swagger petstore example has the array parameter with default field.
I wonder what it means but in protobuf, it should be nil.

"parameters":[{"name":"status","in":"query","description":"Status values that need to be considered for filter","required":true,"type":"array","items":{"type":"string","enum":["available","pending","sold"],"default":"available"},"collectionFormat":"multi"}],

@achew22 achew22 self-assigned this Jan 16, 2017
@t-yuki t-yuki force-pushed the swagger-query-parameters branch from 4d81b7a to 28132c2 Compare January 17, 2017 15:48
Copy link
Collaborator

@achew22 achew22 left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks so much for contributing. Can you comment to kick the CLA bot into gear?

There is a chance that the CLA bot can't handle the case where you're committing on top of someone else's work. If that happens, I will manually merge you from the CLI.

@t-yuki
Copy link
Contributor Author

t-yuki commented Jan 18, 2017

ping @googlebot

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@doroginin
Copy link
Contributor

+1, We need this feature, merge it please!

@achew22
Copy link
Collaborator

achew22 commented Jan 19, 2017

@rgarcia, can you confirm a couple of things for me please before I merge this?

  1. You are the author of the commits that @t-yuki extended upon
  2. You're comfortable with the additional changes made
  3. To the best of your knowledge you've signed the CLA (write query parameters to swagger definition #199 implies yes but I wanna double check)

@achew22 achew22 closed this Jan 19, 2017
@achew22 achew22 reopened this Jan 19, 2017
@rgarcia
Copy link
Contributor

rgarcia commented Jan 19, 2017

@achew22 1. yes 2. yes 3. yes 😄

@achew22
Copy link
Collaborator

achew22 commented Jan 20, 2017

Huh, foiled by the CLI bot. Even on the command line I can't push past this. @yugui can we change the CLAbot to being advisory or non-blocking or whatever github calls it?

@t-yuki
Copy link
Contributor Author

t-yuki commented Jan 20, 2017

Should I re-push without rebase to avoid CLA bot fail?

@achew22
Copy link
Collaborator

achew22 commented Jan 20, 2017

We could be really tricky and merge the other PR then this one quickly as not to break anyone, hopefully. I just sent an email to yugui (not @'d on purpose cause I just sent her an email) about modifying the submission rules. Let's see how that plays out. Meet back here in 24 hours?

@t-yuki t-yuki mentioned this pull request Jan 23, 2017
@yugui
Copy link
Member

yugui commented Jan 25, 2017

@achew22 I don't think we can make the bot advisory. But I have granted admin rights of this project to you and @tmc. So now you can ignore the status check at your own risk.

@tmc tmc merged commit cfee3c5 into grpc-ecosystem:master Jan 25, 2017
@tamalsaha tamalsaha mentioned this pull request Mar 30, 2017
1 task
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
…eters

write query parameters to swagger definition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants