Skip to content

[suggestion] Get apis property from either SystemProperties or additionalProperties #4937

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

Conversation

shybovycha
Copy link
Contributor

@shybovycha shybovycha commented Jan 7, 2020

Introduction

There is one quite nifty but undocumented option, apis, which could be set in system properties. It defines which API groups should be generated. Given the undocumented internal behavior of grouping all operations by tags, this gives an easy way to migrate existing projects to Open-API with minimal changes by automatically generating API definitions and then generating API interfaces with operations automatically grouped according to the existing endpoints.

Hence I am creating a few PRs (#4937, #4938, #4939) addressing this hidden gem of openapi-generator.

This change

I could not really find a good way to pass values through SystemProperties when using the maven plugin. Hence it is pretty much impossible from my point of view to set the apis property, I think that it could also be passed through the additionalProperties option.

Also, see the other PR ( #4938 ) which fixes a bug in parsing the list options in additionalProperties.

Does not seem like a breaking change, hence targeting master.

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.

@shybovycha shybovycha changed the title Get apis property from either SystemProperties or additionalProperties [suggestion] Get apis property from either SystemProperties or additionalProperties Jan 7, 2020
String apiNames = GlobalSettings.getProperty(CodegenConstants.APIS);

if (apiNames == null || StringUtils.isBlank(apiNames)) {
apiNames = (String) opts.getConfig().additionalProperties().get(CodegenConstants.APIS);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this will work… it would mean that generators can no longer support an apis additional property for other reasons as any setting here would cause no API classes to get generated.

Can you provide an example of how you've configured the Maven plugin? I think it would be best to fix system property support there.

@jimschubert
Copy link
Member

@shybovycha I've tested the following, and it works as expected:

diff --git a/modules/openapi-generator-maven-plugin/examples/kotlin.xml b/modules/openapi-generator-maven-plugin/examples/kotlin.xml
index 3a2979232c..a28a2b6095 100644
--- a/modules/openapi-generator-maven-plugin/examples/kotlin.xml
+++ b/modules/openapi-generator-maven-plugin/examples/kotlin.xml
@@ -66,6 +66,10 @@
 							<apiPackage>kotlintest.org.openapitools.client.api</apiPackage>
 							<modelPackage>kotlintest.org.openapitools.client.model</modelPackage>
 							<invokerPackage>kotlintest.org.openapitools.client</invokerPackage>
+
+							<environmentVariables>
+								<apis>Pet</apis>
+							</environmentVariables>
 						</configuration>
 					</execution>
 				</executions>

This generates only PetApi and all associated models. I've looked at the readme for the plugin, and the usage is not clear so I'll get that updated.

@jimschubert
Copy link
Member

Thanks for submitting this PR and for proposing workarounds here and in #4938. I'm going to close this in favor of #5251, which adds an apisToGenerate property to match the existing modelsToGenerate property. It looks like the environmentVariables documentation was missing and wouldn't have been clear that this option was added for defining systemProperties (or, GlobalSettings). The changes in #5251 should clarify how to do this without workaround and fallbacks.

@jimschubert jimschubert closed this Feb 8, 2020
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.

2 participants