[core] GeneratorSettings, WorkflowSettings, and cleanup in CodegenConfigurator #2946
Conversation
|
cc @OpenAPITools/generator-core-team I've filed this to hit master since the changes shouldn't be "public api" (CodegenConfigurator getters and |
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.
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.
This splits settings relevant to generator configuration (the what) and workflow configuration (the how) in an attempt to make configuration easier to conceptualize.
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).
…ng deserialization
* Fix CodegenConfigurator library setting
|
@OpenAPITools/generator-core-team I'm looking for feedback about whether this should target master or the 4.1.0 branch. Working through all usages of Any thoughts one way or another? |
My take is 4.1.0 branch as it's a breaking change with fallbacks |
Good idea for a short name. What about |
* 4.1.x: update petstore samples update to 4.1.0-SNAPSHOT
|
I've updated the target to the 4.1.x branch and added the |
|
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. |
* 4.1.x: (56 commits) sync master Update compatibility table Ruby client: escape path parameters (OpenAPITools#3039) [gradle plugin] Release 4.0.1 fixes (OpenAPITools#3051) Update version to 4.0.2-SNAPSHOT (OpenAPITools#3047) Map number to double time since float is also parsed as double in Qt5 C++ (OpenAPITools#3046) Prepare 4.0.1 release (OpenAPITools#3041) [gradle] Reworking publishing pipeline (OpenAPITools#2886) [typescript-fetch] Fix uploading files (OpenAPITools#2900) Resolves OpenAPITools#2962 - Add properties config to Maven parameters (OpenAPITools#2963) fix(golang): Check error of xml Encode (OpenAPITools#3027) [C++][Restbed] Add handler callback methods (OpenAPITools#2911) Remove null checks for C# value types (OpenAPITools#2933) [python-server] Support python 3.7 for all server-generators (OpenAPITools#2884) Use golang's provided method names (gin) (OpenAPITools#2983) [python] Adding constructor parameters to Configuration and improving documentation (OpenAPITools#3002) Add new option to maven plugin's readme (OpenAPITools#3025) Engine param in maven plugin. (OpenAPITools#2881) Added support for patterns on model properties (OpenAPITools#2948) [csharp] Make API response headers dictionary case insensitive (OpenAPITools#2998) ...
Upgrade Note
to |
PR checklist
./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.shif 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.master,4.1.x,5.0.x. Default:master.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
CodegenConfiguratoras 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 ofCodegenConfiguratorinGenerator.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 callingtoClientOptInputor the newtoContext.This PR introduces two new types:
GeneratorSettingsandWorkflowSettings. 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
CodegenConfiguratormore SOLID, I've also introduced aDynamicSettingstype which handles the deserialization of those settings which were previously consideredCodegenConfigurator. Now,CodegenConfiguratoris simply orchestrating how we configure everything in preparation for generation.DynamicSettingsdoesn'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
GeneratorPropertiesto avoid confusion withGeneratorSettings. The intention is thatGeneratorSettingsare those settings which are applied to a generator (an instance ofCodegenConfig).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 toGlobalSettings.GlobalSettingswill therefore be anything that can be considered outside the responsibility of a generator or its workflow (e.g.debugModelscould be set as a System Property and used by either). We recently had a report of-Doptions not making it down to swagger-parser (see #2706), and this is because the CLI has a custom argument of-Dwhich can easily be confused as the Java property-D. In this PR, I've added a deprecation warning with instructions to stop using the-Dargument togenerateand move this option to System properties. If we find that something like-DdebugModelsor other supported "system properties" currently cause conflicts elsewhere, we may want to add a newglobal-settingsoption. I didn't do this in this PR because it seemed like a little too much for a single change.