Skip to content

[Go] Add multiple servers support to Go(-experimental) client #4635

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

Merged
merged 5 commits into from
Dec 4, 2019

Conversation

jirikuncar
Copy link
Contributor

@jirikuncar jirikuncar commented Nov 28, 2019

Allows the Go(-Experimental) API client to retrieve all server URLs defined in the spec

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09) @bkabrda (2019/07)

related PRs

@jirikuncar jirikuncar changed the title [Go] Add multiple servers support to Go-experimental client [Go] Add multiple servers support to Go(-experimental) client Nov 28, 2019
Copy link
Contributor

@bkabrda bkabrda left a comment

Choose a reason for hiding this comment

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

Great stuff. There are couple minor things that I'd suggest fixing (see the review), but otherwise this is great start to support server variables.
Additionally, this is in line with what other languages (e.g. Python) use, so this is also ok in terms of consistency across the whole project.

@bkabrda
Copy link
Contributor

bkabrda commented Nov 29, 2019

@wing328 hey William 👋 . It seems that this PR is failing in CI because it uses Go 1.12 (released February 2019) feature (strings.ReplaceAll). Is there any documentation available on which go releases we want to support?

Otherwise this PR LGTM, so once we clear the situation with strings.ReplaceAll I will give it a 👍 .

@bkabrda
Copy link
Contributor

bkabrda commented Nov 29, 2019

Ah, please disregard my previous comment, I was looking wrong. This now uses strings.Replace, it just needs to have the samples regenerated after moving to this.

@jirikuncar
Copy link
Contributor Author

Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).

@bkabrda it seems that the PR description template is not completely acurate.

$ find . -iname \*go-\*.sh | grep -v server
./bin/go-experimental-petstore.sh
./bin/openapi3/go-experimental-petstore.sh
./bin/openapi3/go-petstore.sh
./bin/go-petstore-withxml.sh # <- I was missing this one !!!
./bin/go-petstore.sh

I wish there was a Makefile and/or pre-commit hook for this that would check which samples need to be regenerated based on list of edited files.

$ make samples
# or
$ make samples-go samples-go-experimental

Copy link
Contributor

@bkabrda bkabrda left a comment

Choose a reason for hiding this comment

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

Perfect, LGTM now 👍

@jirikuncar
Copy link
Contributor Author

@antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09) @bkabrda (2019/07) I have rebased the PR and regenerated the client code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants