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

[fix] Assign template directory to additional properties #3385

Merged
merged 3 commits into from
Jul 18, 2019

Conversation

jimschubert
Copy link
Member

@jimschubert jimschubert commented Jul 18, 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

As part of settings refactor, I had missed assigning the value of templateDir to additional properties. This result in 4.1.0-SNAPSHOT branch with borked custom template support.

This PR makes that asignment, and fixes some other potential issues with internal state in CodegenConfigurator (set* methods).

Added a unit test which evaluates additional properties exist on a CodegenConfig instance after calling processOpts(). The test is limited to those properties known to be processed by DefaultCodegen.

cc @OpenAPITools/generator-core-team
fixes #3364

@jimschubert jimschubert added this to the 4.1.0 milestone Jul 18, 2019
@auto-labeler
Copy link

auto-labeler bot commented Jul 18, 2019

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@jimschubert
Copy link
Member Author

Bug was introduced in #2946, which was introduced into the 4.1.x branch, so this bug only affects the master SNAPSHOT. I'm removing the bug label since this shouldn't require release notes.

wing328
wing328 previously approved these changes Jul 18, 2019
@wing328 wing328 dismissed their stale review July 18, 2019 05:32

Fat finger in approving

@wing328
Copy link
Member

wing328 commented Jul 18, 2019

AppVeyor (Windows) found the following error:

Time Elapsed 00:00:04.06
mvn clean install --quiet
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
[ERROR] Tests run: 705, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 13.767 s <<< FAILURE! - in TestSuite
[ERROR] shouldSetConfiglProperties(org.openapitools.codegen.config.CodegenConfiguratorTest)  Time elapsed: 0.001 s  <<< FAILURE!
java.lang.AssertionError: expected [C:/Users/appveyor/AppData/Local/Temp/1/test4608376972844168771] but found [C:\Users\appveyor\AppData\Local\Temp\1\test4608376972844168771]
	at org.openapitools.codegen.config.CodegenConfiguratorTest.want(CodegenConfiguratorTest.java:35)
	at org.openapitools.codegen.config.CodegenConfiguratorTest.shouldSetConfiglProperties(CodegenConfiguratorTest.java:106)
[ERROR] Failures: 
[ERROR]   CodegenConfiguratorTest.shouldSetConfiglProperties:106->want:35 expected [C:/Users/appveyor/AppData/Local/Temp/1/test4608376972844168771] but found [C:\Users\appveyor\AppData\Local\Temp\1\test4608376972844168771]
[ERROR] Tests run: 705, Failures: 1, Errors: 0, Skipped: 0
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M3:test (default-test) on project openapi-generator: There are test failures.

@wing328
Copy link
Member

wing328 commented Jul 18, 2019

CircleCI reports the following errors:

Can't load config class with name 'myClientCodegen'
Available:
ada
ada-server
android
apache2
apex
aspnetcore
bash
c
clojure
cwiki
cpp-qt5-client
cpp-qt5-qhttpengine-server
cpp-pistache-server
cpp-restbed-server
cpp-restsdk
cpp-tizen
csharp
csharp-netcore
csharp-dotnet2
csharp-nancyfx
dart
dart-jaguar
eiffel
elixir
elm
erlang-client
erlang-proper
erlang-server
flash
go
go-experimental
go-server
go-gin-server
graphql-schema
graphql-nodejs-express-server
groovy
kotlin
kotlin-server
kotlin-spring
haskell-http-client
haskell
java
jaxrs-cxf-client
java-inflector
java-msf4j
java-pkmst
java-play-framework
java-undertow-server
java-vertx
jaxrs-cxf
jaxrs-cxf-extended
jaxrs-cxf-cdi
jaxrs-jersey
jaxrs-resteasy
jaxrs-resteasy-eap
jaxrs-spec
javascript
javascript-flowtyped
javascript-closure-angular
jmeter
lua
mysql-schema
nodejs-server-deprecated
objc
openapi
openapi-yaml
perl
php
php-laravel
php-lumen
php-slim
php-silex
php-symfony
php-ze-ph
powershell
python
python-experimental
python-flask
python-aiohttp
python-blueplanet
r
ruby
ruby-on-rails
ruby-sinatra
rust
rust-server
scalatra
scala-akka
scala-finch
scala-httpclient-deprecated
scala-gatling
scala-lagom-server
scala-play-server
scalaz
spring
dynamic-html
html
html2
swift2-deprecated
swift3-deprecated
swift4
typescript-angular
typescript-angularjs
typescript-aurelia
typescript-axios
typescript-fetch
typescript-inversify
typescript-jquery
typescript-node
typescript-rxjs
fsharp-giraffe-server

[error] Check the spelling of the generator's name and try again.
ERROR!! FAILED TO RUN ./bin/meta-codegen.sh
Running ./bin/mysql-schema-petstore.sh (output to /dev/null)
Running ./bin/nancyfx-petstore-server-async.sh (output to /dev/null)
Running ./bin/nancyfx-petstore-server.sh (output to /dev/null)

@jimschubert
Copy link
Member Author

@wing328 I can fix the Windows path. I'm not sure why this works in some other tests... Maybe we're just not asserting on the path and it's getting normalized elsewhere.

I don't understand the second error, as I've not done anything with myClientCodegen. I'd need to dig a little deeper into that.

@jimschubert
Copy link
Member Author

I misunderstood the myClientCodegen. I had only tested with setting template directory, and relied on CI for other cases... the default case failed if templateDir wasn't set since it would reassign a null and overwrite a generator's defaults.

We may want to consider having some other "base configuration" tests in which no user options are set on the generator, at least for a single non-custom generator.

@jimschubert
Copy link
Member Author

@wing328 all checks have passed, merging into master now to unblock custom templates in 4.1.0-SNAPSHOT. I'll address anything else that arises in a separate PR.

@jimschubert jimschubert merged commit 944e1c3 into OpenAPITools:master Jul 18, 2019
@jimschubert jimschubert deleted the fix-3364 branch July 18, 2019 13:51
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.

[BUG] Option template directory ignored in 4.1.0-SNAPSHOT
2 participants