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

[core] GeneratorSettings, WorkflowSettings, and cleanup in CodegenConfigurator #2946

Merged
merged 22 commits into from Jun 7, 2019

Conversation

2 participants
@jimschubert
Copy link
Member

commented May 20, 2019

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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.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

This is general enhancements as part of the Separation of Concerns project, and is work toward moving things to a Core package (#845). In doing so, my goal is to separate conceptual responsibilities of our internal implementations.

This PR leaves CodegenConfigurator as an "entrypoint" to configuring internals. There's still some work to be done to simplify that interface so we may remove redundant code from CLI, Maven Plugin, Gradle Plugin, and openapi-generator-online (which duplicates much of CodegenConfigurator in Generator.java).

A potentially minor breaking change, I've removed all getters from CodegenConfigurator. The caller already knows the properties being set on this type, and any evaluation should be done after calling toClientOptInput or the new toContext.

This PR introduces two new types: GeneratorSettings and WorkflowSettings. These are intentionally immutable, which will aid in testing once they are applied to a generator instance (generator authors can evaluate input settings against modified settings to validate any transformations done in the generator). These types are separate so we can clearly differentiate between those settings defining the what (generator outputs) versus the how (the workflow taken to generate outputs).

As part of creating these new Settings types and moving to make CodegenConfigurator more SOLID, I've also introduced a DynamicSettings type which handles the deserialization of those settings which were previously considered CodegenConfigurator. Now, CodegenConfigurator is simply orchestrating how we configure everything in preparation for generation. DynamicSettings doesn't live in the Core module because it relies on Jackson JSON, and there's technically no need for this to live in a user-consumable module (users can just set additional properties directly); loading the config from file is application-specific.

While focusing on this settings cleanup, I wanted to rename GeneratorProperties to avoid confusion with GeneratorSettings. The intention is that GeneratorSettings are those settings which are applied to a generator (an instance of CodegenConfig). GeneratorProperties, on the other hand, is a way to access System properties and modify them on a thread local to avoid race conditions during CI or other workflows… so I've opted to rename this to GlobalSettings. GlobalSettings will therefore be anything that can be considered outside the responsibility of a generator or its workflow (e.g. debugModels could be set as a System Property and used by either). We recently had a report of -D options not making it down to swagger-parser (see #2706), and this is because the CLI has a custom argument of -D which can easily be confused as the Java property -D. In this PR, I've added a deprecation warning with instructions to stop using the -D argument to generate and move this option to System properties. If we find that something like -DdebugModels or other supported "system properties" currently cause conflicts elsewhere, we may want to add a new global-settings option. I didn't do this in this PR because it seemed like a little too much for a single change.

@jimschubert jimschubert added this to In progress in Separation of Concerns via automation May 20, 2019

@OpenAPITools OpenAPITools deleted a comment from auto-labeler bot May 20, 2019

@jimschubert

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

cc @OpenAPITools/generator-core-team I've filed this to hit master since the changes shouldn't be "public api" (CodegenConfigurator getters and GeneratorProperties rename). Everything in this PR should have a pretty clear workaround.

jimschubert added some commits May 16, 2019

Introduce GeneratorSettings + cleanup
GeneratorSettings is an immutable settings object, intended to limit the
manipulation of generator settings.

To move to GeneratorSettings, lots of modification was done to
CodegenConfigurator. The goal  here is that CodegenConfigurator
would create the contextual information required to initiate a
generator run:

* GeneratorSettings
* Workflow related settings
* Configuring "system" GeneratorProperties (ThreadLocal properties)
* Deserializing from file to config object
* Input spec document (OpenAPI, intending to target others)

ClientOpts was generally unused, and the few places it was being used
have been updated to pass the properties to
codegen.additionalProperties.
Add sanity to system properties
The -D argument for the generate command is an application argument
which is easily confused for Java System Properties. This isn't the
case, as setting values here doesn't update the configuration in
System.getProperties().

This adds a warning and deprecation to that option, as defining these
values as system properties will also continue to work as expected. This
makes the -D application argument redundant and confusing.
Contextualize generator/workflow settings
This splits settings relevant to generator configuration (the what) and
workflow configuration (the how) in an attempt to make configuration
easier to conceptualize.
Update Gradle task w/ CodegenConfigurator setters
The 4.0.0 release missed updating the Gradle plugin's target version to
4.0.1-SNAPSHOT, so local builds were targeting the old 4.0.0 API. CI
picked up on this (building against the merge revision w/ target
branch).
Use Paths.get for templateDir/outputDir
* Fix CodegenConfigurator library setting

@jimschubert jimschubert force-pushed the jimschubert:configurator branch from ecd518a to a67b944 May 20, 2019

@jimschubert

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

@OpenAPITools/generator-core-team
@OpenAPITools/openapi-generator-collaborators

I'm looking for feedback about whether this should target master or the 4.1.0 branch.

Working through all usages of -D in scripts, I've found that they were being used as a shortcut for --additional-properties. I don't think this was ever an intended use case. I still think we'd need to get rid of the -D argument to the generate command because it's easily confused with the -D argument to java for setting system properties. I'd considered adding a -P short name for --additional-properties, which I think would more clearly indicate that these are properties rather than defined system properties.

Any thoughts one way or another?

@wing328

This comment has been minimized.

Copy link
Member

commented May 22, 2019

I'm looking for feedback about whether this should target master or the 4.1.0 branch.

My take is 4.1.0 branch as it's a breaking change with fallbacks

@wing328

This comment has been minimized.

Copy link
Member

commented May 22, 2019

I'd considered adding a -P short name for --additional-properties

Good idea for a short name. What about -p (lower case) instead as all the other short names are lower case too?

@jimschubert jimschubert changed the base branch from master to 4.1.x May 23, 2019

jimschubert added some commits May 24, 2019

Merge branch '4.1.x' into configurator
* 4.1.x:
  update petstore samples
  update to 4.1.0-SNAPSHOT
@jimschubert

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

I've updated the target to the 4.1.x branch and added the -p option as a shortcut for --additional-properties. I've also regenerated samples.

@jimschubert

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

I've submitted a final commit, there were a couple of objc scripts which passed on my machine but failed on Shippable with the same syntax (? This is probably an IFS expansion issue).

Builds are passing now. Sorry for the tons of file changes.

Merge branch '4.1.x' into configurator
* 4.1.x: (56 commits)
  sync master
  Update compatibility table
  Ruby client: escape path parameters (#3039)
  [gradle plugin] Release 4.0.1 fixes (#3051)
  Update version to 4.0.2-SNAPSHOT (#3047)
  Map number to double time since float is also parsed as double in Qt5 C++ (#3046)
  Prepare 4.0.1 release (#3041)
  [gradle] Reworking publishing pipeline (#2886)
  [typescript-fetch] Fix uploading files (#2900)
  Resolves #2962 - Add properties config to Maven parameters (#2963)
  fix(golang): Check error of xml Encode (#3027)
  [C++][Restbed] Add handler callback methods (#2911)
  Remove null checks for C# value types (#2933)
  [python-server] Support python 3.7 for all server-generators (#2884)
  Use golang's provided method names (gin) (#2983)
  [python] Adding constructor parameters to Configuration and improving documentation (#3002)
  Add new option to maven plugin's readme (#3025)
  Engine param in maven plugin. (#2881)
  Added support for patterns on model properties (#2948)
  [csharp] Make API response headers dictionary case insensitive (#2998)
  ...

@jimschubert jimschubert merged commit a96ab1c into OpenAPITools:4.1.x Jun 7, 2019

4 checks passed

Shippable Run 8531 status is SUCCESS.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Separation of Concerns automation moved this from In progress to Done Jun 7, 2019

@jimschubert jimschubert deleted the jimschubert:configurator branch Jun 7, 2019

@wing328 wing328 added this to the 4.0.2 milestone Jun 20, 2019

@wing328

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Upgrade Note

-D to pass generator options has been deprecated. Please use --additional-properties instead, for example, from:

ags="$ags -DprojectName=Petstore --model-package Samples.Petstore"

to

ags="$ags --additional-properties projectName=Petstore --model-package Samples.Petstore"

jmini added a commit to jmini/openapi-generator that referenced this pull request Jul 10, 2019

jimschubert added a commit that referenced this pull request Jul 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.