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

[CLI] Initial implementation for batch generation #3789

Merged
merged 25 commits into from Oct 9, 2019

Conversation

@jimschubert
Copy link
Member

commented Aug 29, 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 A WIP Requesting feedback before I convert all exiting JSON based configs which are executed by ensure-up-to-date

Allows for generating multiple outputs via config. Just specify multiple config files on command line.

Intent for this is to reduce CI times to generate outputs as well as to reduce time for users to run ensure-up-to-date to meet PR standards.

Example command:

openapi-generator batch --includes-base-dir `pwd` --fail-fast  -- bin/ci/*

As part of this implementation, the batch command support a customized JSON key, !include. If this key's value refers to an existing file, that file's contents are "unwrapped" into the config during deserialization. This allows us to easily point to the same configs used by our sample scripts without modifying the CLI generate task's switches or assumptions.

cc @OpenAPITools/generator-core-team

Allows for generating multiple outputs via config. Just specify multiple
config files on command line.

Intent for this is to reduce CI times to generate outputs as well as to
reduce time for users to run ensure-up-to-date to meet PR standards.

Example command:

  openapi-generator batch --includes-base-dir `pwd` --fail-fast  -- bin/ci/*

---

As part of this implementation, the batch command support a customized
JSON key, `!include`. If this key's value refers to an existing file,
that file's contents are "unwrapped" into the config during
deserialization. This allows us to easily point to the same configs used
by our sample scripts without modifying the CLI generate task's switches
or assumptions.
@jimschubert

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2019

If it's ever needed again… here's the small program I used to generate JSON from existing bash scripts: https://github.com/jimschubert/openapi-generator-commands-processor

if (null != threads && (threads > 0 && threads < Thread.activeCount())) {
numThreads = threads;
}

This comment has been minimized.

Copy link
@wing328

wing328 Sep 29, 2019

Member

Minor suggestion: maybe good to use LOGGER.info to show how many threads are used as an FYI to the user.

@wing328

This comment has been minimized.

Copy link
Member

commented Sep 29, 2019

I believe you will update the doc (https://openapi-generator.tech/docs/usage) later as part of the upcoming release

@wing328

This comment has been minimized.

Copy link
Member

commented Sep 29, 2019

@jimschubert Overall it looks pretty good 👍Did simple tests locally and worked as expected. The CircleCI test failed with a weird error I've never seen. I"ve just restarted the job to see if the issue goes away.

@wing328

This comment has been minimized.

Copy link
Member

commented Sep 29, 2019

Some Petstore Play server files have changed ...

Perform git status
On branch batch-generation
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   samples/server/petstore/java-play-framework-async/app/controllers/PetApiController.java
	modified:   samples/server/petstore/java-play-framework-async/app/controllers/PetApiControllerImp.java
	modified:   samples/server/petstore/java-play-framework-async/app/controllers/PetApiControllerImpInterface.java
	modified:   samples/server/petstore/java-play-framework-async/app/controllers/StoreApiController.java
	modified:   samples/server/petstore/java-play-framework-async/app/controllers/StoreApiControllerImp.java
	modified:   samples/server/petstore/java-play-framework-async/app/controllers/StoreApiControllerImpInterface.java
	modified:   samples/server/petstore/java-play-framework-async/app/controllers/UserApiController.java
	modified:   samples/server/petstore/java-play-framework-async/app/controllers/UserApiControllerImp.java
	modified:   samples/server/petstore/java-play-framework-async/app/controllers/UserApiControllerImpInterface.java

Is that due to mismatch in the script (play server) and the corresponding JSON files under ./bin/ci ?

@jimschubert

This comment has been minimized.

Copy link
Member Author

commented Sep 30, 2019

@wing328 that could be due to CI running against the merge commit. I'd need to merge master into this branch and regenerate samples to verify.

@jimschubert

This comment has been minimized.

Copy link
Member Author

commented Sep 30, 2019

Regarding doc update, if this looks good and we decide to use it... I'll include doc update on this PR.

jimschubert added 3 commits Sep 30, 2019
* master: (110 commits)
  [golang] Regenerate all go samples  (#3988)
  Better tests for string (number) (#3953)
  Add missing enum processing in C++ codegen, already present for Qt5 (#3986)
  [C++] [Pistache] Removed deprecated warnings (#3985)
  [C++][Pistache] Simplified model template (#3417)
  add go oas3 petstore to ensure up-to-date (#3979)
  replace gitter with slack in the doc (#3977)
  Fix wrong variable name in LessThan and LessThanOrEqual asserts (#3971)
  #3957 - Removed hardcoded baseUrl (#3964)
  Regenerate go openapi3 samples (#3975)
  [rust] Make it easier to test rust client generator (#3543)
  Fix issue3635 (#3948)
  add gradle repository (#3867)
  [java] allow to use setArtifactVersion() programmatically (#3907)
  Add a link to DevRelCon SF 2019 (#3961)
  Add a link to a medium blog post (#3960)
  update maven-compiler-plugin version (#3956)
  fix generateAliasAsModels in default generator (#3951)
  Implement BigDecimal to Decimal in swift4 for currency data as type=string format=number (#3910)
  Add F# Functions server generator (#3933)
  ...
@wing328

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

that could be due to CI running against the merge commit.

@jimschubert I don't think CircleCI does that but I could be wrong. From what I know only shippable and Appveyor will try to test against the merge commit.

Anyway, it's still good to merge the latest master into this branch. I'll do it later today to see if the issue goes away.

jimschubert added 6 commits Oct 6, 2019
* master: (35 commits)
  [haskell-http-client] update samples (#4073)
  [haskell-http-client] Bump deps to LTS 14.7 (#4068)
  update release for 4.2.0
  [typescript-axios] Fix api generating incorrect seralization type check (#4051)
  prepare 4.1.3 release (#4052)
  typescript-node: form data file (#3967)
  Add a link to blog post on vertx and openapi (#4048)
  better wording for apiNameSuffix option description (#4045)
  [Ruby] fix ruby test, update error message (#4041)
  [PHP] Correctly format JSON in headers (#4024)
  [haskell-http-client] add dateTimeParseFormat cli option - overrides the format string used to parse a datetime (#4037)
  Add frankyjuang to the C# technical committee (#4036)
  Feature/api name suffix (#3918)
  [F#] minor improvements to the generators (#3968)
  Repaired Checkstyle (#4029)
  mockito 3.1.0 (#4035)
  typescript-fetch: fix return type of primitive value (#4028)
  [typescript][node]: Add accept header if produces is not empty (#3966)
  [haskell-http-client] disable unused import warning in Core.hs (#4020)
  Add a link to the tutorial in http4k (#4019)
  ...
@jimschubert

This comment has been minimized.

Copy link
Member Author

commented Oct 6, 2019

I believe you will update the doc (https://openapi-generator.tech/docs/usage) later as part of the upcoming release

@wing328 the usage page is all "public" commands. The batch generation command is hidden, and I think it should remain CI-only until we work out any kinks. For example, I'm wondering if we should change all shell scripts directly under ./bin to use externalized configs. If we did this, we could merge the ./bin/ci/*.json contents with the relevant configs under ./bin/*.json (excluding the !include customization). The only problem there would be in cases where ./bin/*.json files are shared across sample outputs for generators with different options applied, such as the ruby client.

I'm thinking about incorporating this into the CI scripts as a separate PR. This will require that we run the ./bin/meta-codegen.sh script separately.

@jimschubert jimschubert merged commit 54d7e8c into master Oct 9, 2019
8 checks passed
8 checks passed
Shippable Run 11036 status is SUCCESS.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr Build is passing
Details
continuous-integration/drone/push Build is passing
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@wing328 wing328 added this to the 4.2.0 milestone Oct 10, 2019
jimschubert added a commit that referenced this pull request Oct 14, 2019
* master: (78 commits)
  Replaced dashes with underscores in build.gradle files. (#4092)
  [cxf-cdi] use @FormParam for form parameters when it is not Multipart (#4125)
  Corrections to script names (#4135)
  [python] Add missing keywords python (#4134)
  Update PULL_REQUEST_TEMPLATE.md (#4080)
  revert the fix to broken links
  Rename property name from propertyRawName to propertyBaseName (#4124)
  [Go] Fix go.mod and go.sum for 1.13 (#4084)
  [kotlin] add option for non public api (#4089)
  Added new discriminator RawName property to preserve declared discriminator for @JsonTypeInfo annotations (#3320)
  Fix links to other files (#4120)
  [JAVA][JAXRS] Fix parameters validation (#3862)
  Make Resttemplate thread safe by using the withHttpInfo pattern used by many other generated clients (#4049)
  Disabling linting for typescript-fetch (#4110)
  [Kotlin][Client] fix missing curly bracket when the model contains enum property (#4118)
  Fix NPE in Elm path parameter (#4116)
  test aiohttp first (#4117)
  add back ruby client folders
  update petstore samples
  [CLI] Initial implementation for batch generation (#3789)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.