Move bash argv opt to end of ags line in scripts #124

jimschubert · 2018-05-22T04:26:45Z

The $@ option in bash doesn't make sense to come before generate
because the only option we can pass before generate cli usage is help.

System properties can be passed via JAVA_OPTS, so there's not really a
need for any intermediaries in the command line construction.

Having $@ at the end of the arguments list allows maintainers and users
inspecting options to quickly pass new options to a script. For example,

./bin/aspnetcore-petstore.sh --additional-properties sourceFolder=asdf

For command line arguments that may appear more than once in the
arguments list, this change doesn't provide any rules about overwriting
values that may exist (hard-coded) in the script. That is, in the
example above, if aspnetcore-petstore.sh already includes the
sourceFolder set to a different value, the "winning" value is up to the
options parser and openapi-generator-cli implementation.

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 and ./bin/security/{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\.
Filed the PR against the correct branch: Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

The $@ option in bash doesn't make sense to come before `generate` because the only option we can pass before generate cli usage is `help`. System properties can be passed via JAVA_OPTS, so there's not really a need for any intermediaries in the command line construction. Having $@ at the end of the arguments list allows maintainers and users inspecting options to quickly pass new options to a script. For example, ``` ./bin/aspnetcore-petstore.sh --additional-properties sourceFolder=asdf ``` For command line arguments that may appear more than once in the arguments list, this change doesn't provide any rules about overwriting values that may exist (hard-coded) in the script. That is, in the example above, if aspnetcore-petstore.sh already includes the sourceFolder set to a different value, the "winning" value is up to the options parser and openapi-generator-cli implementation.

jimschubert · 2018-05-22T04:32:20Z

This is a lot of files changed, but basically the same change in almost every file (some java/jaxrs files have multiline ags).

I used a recorded macro to make these changes, then verified I had made no mistakes with variations of git-grep on staged files:

 git grep --break --heading --cached '[^"]generate' **/*.sh
 git grep --break --heading --cached '$@[^"]' **/*.sh
 git grep --break --heading --cached ' $@[^"]' **/*.sh
 git grep --break --heading --cached '[^"]generate ' **/*.sh
 git grep --break --heading --cached '$@' **/*.sh

I don't know if this helps in evaluation, but I assume you could pull the branch and target the HEAD commit with git grep to double-check.

jmini · 2018-05-22T06:09:17Z

Looks good to me.

wing328

Thanks for the PR to update all the scripts. (I only updated them one by one when I'm testing the generator)

jmini · 2018-05-22T14:25:22Z

bin/jaxrs-cxf-cdi-petstore-server.sh

@@ -27,6 +27,6 @@ fi

 # if you've executed sbt assembly previously it will use that instead.
 export JAVA_OPTS="${JAVA_OPTS} -XX:MaxPermSize=256M -Xmx1024M -DloggerPath=conf/log4j.properties"
-ags="$@ generate -t modules/openapi-generator/src/main/resources/JavaJaxRS/cxf-cdi -i modules/openapi-generator/src/test/resources/2_0/petstore.yaml -l jaxrs-cxf-cdi -o samples/server/petstore/jaxrs-cxf-cdi -DhideGenerationTimestamp=true"
+="generate -t modules/openapi-generator/src/main/resources/JavaJaxRS/cxf-cdi -i modules/openapi-generator/src/test/resources/2_0/petstore.yaml -l jaxrs-cxf-cdi -o samples/server/petstore/jaxrs-cxf-cdi -DhideGenerationTimestamp=true $@"


ags is missing. The line should be ags="generate -t modules/...

jmini · 2018-05-22T14:27:32Z

This PR breaks some script. I found the first one bin/jaxrs-cxf-cdi-petstore-server.sh .

@jimschubert can you have a look and submit an other PR to fix the scripts?

See OpenAPITools#124

* jaxrs-cxf-cdi: fix model enum * Fix script 'bin/jaxrs-cxf-cdi-petstore-server.sh' See #124 * Run 'bin/jaxrs-cxf-cdi-petstore-server.sh'

* master: (36 commits) jaxrs-cxf-cdi: fix outer enum (OpenAPITools#131) Move bash argv opt to end of ags line in scripts (OpenAPITools#124) Reduce CI logging (OpenAPITools#119) Download elm dependencies without prompting user. (OpenAPITools#118) [aspnetcore] Make the use of Swashbuckle optional (OpenAPITools#110) DefaultGenerator: fix NullPointerException (OpenAPITools#109) [Java] use html entities in javadoc of generated code (OpenAPITools#106) Update PULL_REQUEST_TEMPLATE.md [java] Enum in array of array (OpenAPITools#66) Rename datatype to dataType in CodegenProperty (OpenAPITools#69) update elm test to compile all elm files (OpenAPITools#95) Fix Petstore example for Elm (OpenAPITools#96) Update Docker documentation (OpenAPITools#97) CaseFormatLambda has been added, params for Rest-assured client has been refactored (OpenAPITools#91) Update integration.md [Clojure] Add util method to set the api-context globally (OpenAPITools#93) [JaxRS-Java] issue with implFolder on windows, and required fields generation for containers (OpenAPITools#88) Set parameters allowableValues dynamically (OpenAPITools#65) Meta: set version for "build-helper-maven-plugin" (OpenAPITools#89) Fix javadoc issues in "openapi-generator" module (OpenAPITools#84) ...

jimschubert · 2018-05-23T12:48:40Z

@jmini thanks for finding that script. I had pushed to this PR branch, assuming CI would fail if I missed anything.

The broken script was fixed by your merge of #131 and the CI script was made to work as expected by #57 and #133.

…est-0.x chore(deps): update dependency type-fest to v0.19.0

jimschubert added the Enhancement: CI/Test label May 22, 2018

jmini approved these changes May 22, 2018

View reviewed changes

wing328 approved these changes May 22, 2018

View reviewed changes

wing328 merged commit 8df3e54 into OpenAPITools:master May 22, 2018

jmini mentioned this pull request May 22, 2018

Cli generator name option, replaces 'language' options in CLI and Maven Plugin #57

Merged

4 tasks

jmini reviewed May 22, 2018

View reviewed changes

This was referenced May 22, 2018

Update samples scripts to move args ($@) to end of script text #113

Closed

CLI: Exit status should be 1 when no/wrong arguments are passed #132

Closed

jmini added a commit to jmini/openapi-generator that referenced this pull request May 22, 2018

Fix script 'bin/jaxrs-cxf-cdi-petstore-server.sh'

fbfc2d8

See OpenAPITools#124

jimschubert pushed a commit that referenced this pull request May 22, 2018

jaxrs-cxf-cdi: fix outer enum (#131)

b73ab02

* jaxrs-cxf-cdi: fix model enum * Fix script 'bin/jaxrs-cxf-cdi-petstore-server.sh' See #124 * Run 'bin/jaxrs-cxf-cdi-petstore-server.sh'

jimschubert deleted the scripts-arguments-update branch May 23, 2018 12:48

wing328 added this to the 3.0.0 milestone Jun 17, 2018

nilskuhn pushed a commit to nilskuhn/openapi-generator that referenced this pull request Apr 6, 2023

Merge pull request OpenAPITools#124 from OpenAPITools/renovate/type-f…

483eaa1

…est-0.x chore(deps): update dependency type-fest to v0.19.0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move bash argv opt to end of ags line in scripts #124

Move bash argv opt to end of ags line in scripts #124

jimschubert commented May 22, 2018

jimschubert commented May 22, 2018

jmini commented May 22, 2018

wing328 left a comment

jmini May 22, 2018 •

edited

Loading

jmini commented May 22, 2018

jimschubert commented May 23, 2018 •

edited

Loading

Move bash argv opt to end of ags line in scripts #124

Move bash argv opt to end of ags line in scripts #124

Conversation

jimschubert commented May 22, 2018

PR checklist

jimschubert commented May 22, 2018

jmini commented May 22, 2018

wing328 left a comment

Choose a reason for hiding this comment

jmini May 22, 2018 • edited Loading

Choose a reason for hiding this comment

jmini commented May 22, 2018

jimschubert commented May 23, 2018 • edited Loading

jmini May 22, 2018 •

edited

Loading

jimschubert commented May 23, 2018 •

edited

Loading