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

Added server variable support #816

Merged
merged 2 commits into from
Aug 19, 2018

Conversation

TiFu
Copy link
Contributor

@TiFu TiFu commented Aug 14, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.3.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

See #793

I implemented a solution similar to authMethods in supportingFiles JSON (see generated json below).

Spec: https://gist.github.com/TiFu/0934b362f25c66bc2b758f33e15520fc
Generated Supporting Files JSON: https://gist.github.com/TiFu/919a8888837e4e24fd32a05dc8d289f9

Generated under the root object (use -DdebugSupportingFiles to print the full mustache template used):

 "servers" : [ {
    "url" : "http://petstore.swagger.io:{port}/{basePath}",
    "description" : "The production API server",
    "variables" : [ {
      "name" : "port",
      "defaultValue" : "8443",
      "enumValues" : [ "8443", "443" ]
    }, {
      "name" : "basePath",
      "defaultValue" : "v2"
    } ]
  }, {
    "url" : "http://petstore.swagger.io",
    "description" : "The production API server",
    "variables" : [ ]
  } ],

Question

Do I have to regenerate any samples? This is a non-breaking change because I only added a new property to the JSON (which is passed to mustache) for supportingFiles.

CC @wing328 @jmini

@wing328
Copy link
Member

wing328 commented Aug 15, 2018

@TiFu agreed with you that it's a non-breaking change and there's no need to regenerate the samples as the test spec does not have anything related to server variables at the moment.

I'll review later today and let you know if I've any question.

@jmini
Copy link
Member

jmini commented Aug 15, 2018

Do I have to regenerate any samples?

No. (to prove it: Shippable Build is green).

This is a non-breaking change because I only added a new property to the JSON (which is passed to mustache) for supportingFiles.

I agree.


Can you share some insight about how this is used?

Do you plan to update some generator/template to make usage of this new value?

@jmini
Copy link
Member

jmini commented Aug 15, 2018

Can you share some insight about how this is used?
Do you plan to update some generator/template to make usage of this new value?

See #793 (comment)

@TiFu
Copy link
Contributor Author

TiFu commented Aug 15, 2018

I'm working on merging the TS generators into one generator.

My plan is to generate configurable server objects:

class BaseServer<T> {
           public constructor(private url: string, private variables: T) {
        
            }
            public set(variable: keyof T, value: string) {
                 this.variables[variable] = value;
            }
}

let devServer = new Server<{ "port": string, "basepath": string }>(""http://petstore.swagger.io:{port}/{basePath}"", { "port": "8443", "basepath": "v2"});

The server objects are passed to the different operations (either via a default config object or as an argument in the method call for the operation):

let petApi = new PetAPI();
petApi.addPet("myPet", devServer);

@wing328
Copy link
Member

wing328 commented Aug 16, 2018

@TiFu for the suggested use case (TS), what about passing devServer during object instantiation of PetAPI instead?

let devPetApi = new PetAPI(devServer);
devPetApi.addPet("myPet");

let petApi = new PetAPI(); // default to prod server
petApi.addPet("myPet");

@TiFu
Copy link
Contributor Author

TiFu commented Aug 16, 2018

@wing328 that's probably a better idea - the implementation I currently had in mind for the ts generators actually uses a configuration object for the APIs which contains more parameters than just the server (e.g. authMethods configured with the necessary credentials e.g. api key)

@wing328
Copy link
Member

wing328 commented Aug 16, 2018

uses a configuration object for the APIs which contains more parameters than just the server (e.g. authMethods configured with the necessary credentials e.g. api key)

https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore-security-test/typescript-fetch/configuration.ts seems to be a good starting point.

@wing328
Copy link
Member

wing328 commented Aug 17, 2018

If no further question/feedback on this PR, I'll merge it over the weekend.

@wing328 wing328 merged commit 3b9de3b into OpenAPITools:master Aug 19, 2018
jimschubert added a commit that referenced this pull request Aug 21, 2018
* master:
  Fix problems in typescript jquery generator (#801)
  [PHP] Add gitignore to AbstractPhpCodegen (#765)
  Improve documentation for usage of a generator in an other jar (#817)
  Improve Symfony 4.1 compatibility (#830)
  📝 Updating 'help generate' switches in README
  [cli] Don't log to STDOUT if debug flags are set (#474)
  [gradle-plugin] README notes on multiple specs (#847)
  Added server variable support (#816)
  fix erlang optiona/required parameters (#829)
  [Java][Rest-assured] Fix generated javadoc and "swagger-annotations" improvement (#831)
@wing328
Copy link
Member

wing328 commented Aug 22, 2018

@TiFu thanks for the enhancement, which is included in the v3.2.2 stable release: https://twitter.com/oas_generator/status/1032252335131512832

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* Added server variable support

* Replaced tabs with 4 spaces
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.

None yet

3 participants