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

[ci][cli] Moving ensures script to config-based batch generation of samples #6509

Merged
merged 40 commits into from
Jun 9, 2020

Conversation

jimschubert
Copy link
Member

@jimschubert jimschubert commented Jun 1, 2020

All samples scripts are now config based with a single bash entry point: bin/generate-samples.sh.

File cleanup using the FILES file will be done later.

Elm is currently skipped as it requires that we skip validation of the spec, so will need to be configured.

Running against the 179 configurations in bin/configs I see the following stats:

[SUCCESS] Batch generation completed successfully.
./bin/generate-samples.sh  54.20s user 6.64s system 243% cpu 24.954 total

This should address conversation in #6333.

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.

@auto-labeler auto-labeler bot added OpenAPI Generator CLI WIP Work in Progress labels Jun 1, 2020
@auto-labeler
Copy link

auto-labeler bot commented Jun 1, 2020

👍 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.

@spacether
Copy link
Contributor

spacether commented Jun 1, 2020

What is the other folder name meant to tell us or mean?
https://github.com/OpenAPITools/openapi-generator/tree/ensures/bin/configs/other

@spacether
Copy link
Contributor

How about changing the name of the configs folder to something like samples_configs to convey that these are the config files for sample generation? configs is very generic and samples_configs helps people understand that this folder exists to configure our sample generation.

@jimschubert
Copy link
Member Author

I'll write up more when this isn't a wip. But "other" are those that aren't in the ensure-up-to-date script in current master.

As for the suggested rename of the folder, I don't think that's necessary. When I'm done and I've updated docs, it'll be obvious enough what these configs are, especially with the naming of each yaml file.

* master:
  Update Generate.java (#6515)
  Undo PR #6451 (#6514)
  Minor enhancement to Python client generator's code format (#6510)
  [python-experimental] Quicken package loading (#6437)
  [Python][Client] Fix delimiter collision issue #5981 (#6451)
  [Java][Jersey2] add petstore integration tests (#6508)
  UE4 client generator fixes (#6438)
  Fix docs typos (#6478)
  [php-laravel] Show required PHP version in docs (#6502)
  [php-lumen] Show required PHP version in docs (#6501)
  [Java][Jersey2] Fix typo and script, Log enhancements, HTTP signature, deserialization (#6476)
  Remove deprecations 5.0 (#6060)
Generators support .openapi-generator-ignore, allowing maintainers to
explicitly ignore the regeneration of files which have been modified.

Note that the tooling does not overwrite test files whenever those files
exist, and it's not entirely necessary to add test files to the ignore
file.
* master:
  [Go][Experimental] Fix discriminator lookup (#6521)
  Typescript-rxjs: print param name (#6368)
  add oneof discrimistrator lookup to go experimental (#6517)
  [PowerShell] Add useOneOfDiscriminatorLookup option (#6516)
  add discriminator support to anyOf powershell client (#6512)
  [Go][Experimental] Add discriminator support to anyOf (#6511)
* master:
  Fix typescript generator for parameter collectionFormat for pipes ssv (#6553)
  [C++][Pistache] Catch HttpError from user-provided handler (#6520)
  remove scala related profile from the pom (#6554)
  move ruby tests to travis (#6555)
  [Java][jersey2] fix cast error for default value in DateTimeOffset object (#6547)
  [Swift] fix GET request with array parameter (#6549)
  [kotlin][spring] Fix ApiUtil compilation (#6084)
  update python samples
  [Python] Fixed docstrings in api.mustache (#6391)
  [BUG][python] Support named arrays (#6493)
  [Go] whitelist AdditionalProperties in the field name (#6543)
  [kotlin][client] remove tabs usage (#6526)
  [PS] automatically derive discriminator mapping for oneOf/anyOf  (#6542)
  [C++][Ue4] various bus fixes (#6539)
  Fix incorrect npx command (#6537)
  update pester to 5.x (#6536)
  comment out openapi3 java jersey2-java8 tests
  add additional properties support to powershell client generator (#6528)
  [Go][Experimental] Support additionalProperties (#6525)
  #5476 [kotlin] [spring] fix swagger and spring annotation for defaultValue (#6101)
@jimschubert
Copy link
Member Author

Regarding the two TODO items... swift checks will have to be post-merge. Also for appveyor, we may consider using the following GitHub workflow so we have consistency in how we execute the script via bash:

name: Build
on:
  push:
    branches:  
      - master
  pull_request:
    branches:
      - master

jobs:
  build:
    name: Build on JDK ${{ matrix.java }} and ${{ matrix.os }}
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        java: [8, 11]
        os: [ubuntu-latest, windows-latest]
        
    steps:
      - name: Check out code
        uses: actions/checkout@v2

      - name: Set up JDK ${{ matrix.java }}
        uses: actions/setup-java@v1 
        with:
          java-version: ${{ matrix.java }}
          
      - uses: actions/cache@v1
        with:
          path: ~/.m2/repository
          key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}
          restore-keys: |
            ${{ runner.os }}-maven-
        
      - name: Build with Maven
        shell: bash
        run: mvn -nsu -B --quiet -Djacoco.skip=true -Dorg.slf4j.simpleLogger.defaultLogLevel=error --no-transfer-progress clean install --file pom.xml ${{ matrix.flags }}
        
      - name: Test gradle
        shell: bash
        run: gradle -b modules/openapi-generator-gradle-plugin/samples/local-spec/build.gradle buildGoSdk --stacktrace

      - name: Upload Maven build artifact
        uses: actions/upload-artifact@v1
        if: matrix.java == '8'
        with:
          name: artifact
          path: modules/openapi-generator-cli/target/openapi-generator-cli.jar

  verify:
    name: Verifies integrity of the commit on ${{ matrix.os }}
    needs: build
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        os: [ubuntu-latest, windows-latest]
    steps:
      - name: Check out code
        uses: actions/checkout@v2
      - name: Download build artifact
        uses: actions/download-artifact@v1
        with:
          name: artifact
      - name: Run Ensures Script
        run: |
          mkdir -p modules/openapi-generator-cli/target/
          mv artifact/openapi-generator-cli.jar modules/openapi-generator-cli/target/
          ./bin/utils/ensure-up-to-date

I've tested this in a private repository against this branch and it takes about 12 minutes to:

  • Build Java 8 (Linux)
    • smoke test generation via gradle
    • (for Java 8 only) Cache the jar for use in the verification step
  • Build Java 8 (Windows)
    • smoke test generation via gradle
  • Build Java 11 (Linux)
    • smoke test generation via gradle
  • Build Java 11 (Windows)
    • smoke test generation via gradle
  • Run ensure-up-to-date (Linux)
  • Run ensure-up-to-date (Windows)

The 4 build jobs run in parallel, once completed the verify job runs Linux+Windows in parallel.

As a separate verification, we could potentially move a couple sample tests here such as Go and Swift.

Copy link
Contributor

@spacether spacether left a comment

Choose a reason for hiding this comment

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

Looks good to me

@wing328
Copy link
Member

wing328 commented Jun 8, 2020

Overall it looks good 👍

Did some tests locally (mac, windows) and here are 3 minor feedback:

  • In https://github.com/OpenAPITools/openapi-generator/pull/6509/files#diff-195a635ad245ded9076330e31134bd80R8, I would suggest adding a line or 2 to ask Windows users install Git BASH as all the batch files are gone (Git bash works for me under Windows to run the generate script under ./bin/ to update samples) so we need put down some instructions to guide Windows users on how to update the sample.
  • personally I prefer the generate script shows what files have been generated and also some generators will show useful information such as file post-processing, which will be helpful to new users/contributors.
  • the script will wait for 5 seconds no matter what so I'll see if I can file a PR later to skip the 5-second wait time if there's at least one argument provided (so not updating all samples) as I update the samples a lot :)

@wing328
Copy link
Member

wing328 commented Jun 8, 2020

I'll try to address the bitrise.io CI failure tomorrow and merge if no further questions/feedback on this PR.

@wing328 wing328 merged commit 60ceded into master Jun 9, 2020
@wing328 wing328 deleted the ensures branch June 9, 2020 10:29
@wing328
Copy link
Member

wing328 commented Jun 9, 2020

I'll file follow-up PRs for what I mentioned above and also for some recent changes (e.g. rust client petstore)

@jimschubert
Copy link
Member Author

@wing328 regarding

personally I prefer the generate script shows what files have been generated and also some generators will show useful information such as file post-processing, which will be helpful to new users/contributors.

I guess if the input to the script is a single yaml, we could revert to calling generate rather than batch. I think that information is too verbose when running more than one generator at a time.

jimschubert added a commit that referenced this pull request Jun 9, 2020
* master:
  [ci][cli] Moving ensures script to config-based batch generation of samples (#6509)
  Add Instana to the list of users (#6584)
  [rust][reqwest] fix broken export (#6586)
@wing328 wing328 added this to the 5.0.0 milestone Jul 3, 2020
Patouche added a commit to Patouche/openapi-generator that referenced this pull request Aug 26, 2020
wing328 pushed a commit that referenced this pull request Aug 31, 2020
…on Nullable library (#6154)

* Add option to prevent usage of jackson-nullable (#2901)

Add a option for all java client and server to prevent
usage of third party library (jackson-databind-nullable)
which may be forbidden in some company

Add samples for Vertx, Spring MVC, Spring Cloud, Feign and Play

Upgrade dependencies for org.openapitools:jackson-databind-nullable

* Samples - Remove dependency org.openapitools:jackson-databind-nullable (#2901)

* Fix generation of gradle file for vertx (#2901)

* Regenerate samples (#2901)

* Fix documentation and up to date (#2901)

* Fix forgotten regeneration of vertx after dependency integration (#2901)

* Regenerate template after rebase (#2901)

* Use yaml config files introduce in #6509 to manage samples (#2901)

* Regenerate template using the config (#2901)

* Fix bad version during testing generated samples (#2901)

* Regenerate template after fix bad version (#2901)

* Fix merge, allow for set importing on codegen model

Co-authored-by: Jim Schubert <james.schubert@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants