Skip to content

Recreate petstore samples #4458

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

Conversation

roxspring
Copy link
Contributor

@roxspring roxspring commented Nov 12, 2019

Recreated samples using latest master to get a clean baseline ahead of #4454.

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.

@roxspring roxspring marked this pull request as ready for review November 12, 2019 10:31
@roxspring
Copy link
Contributor Author

Note that there are no code or input changes here, just running *-petstore.sh. Presumably CI breakages are due to existing state of master 😢

Not targeting a particular programming language... does that mean I should tag the core team for review? @wing328 @jimschubert @cbornet @ackintosh @jmini @etherealjoy

@jimschubert
Copy link
Member

@roxspring I've opened and reviewed a few PRs this weekend, and the only thing I've found is that I was missing elm-format locally. I pinged the core team recommending that we remove language specific formatters from these scripts.

Master is currently green, and other PRs are green. The amount of changes in this PR is unexpected, and in some cases (like the bash client) the changes aren't valid. Can you rebase master, rebuild the project, then run bin/utils/ensure-up-to-date again?

@jimschubert
Copy link
Member

jimschubert commented Nov 12, 2019

For example, branching off current master you should see only the following for the bash client:

diff --git a/samples/client/petstore/bash/.openapi-generator/VERSION b/samples/client/petstore/bash/.openapi-generator/VERSION
index c3a2c7076f..d168f1d8bd 100644
--- a/samples/client/petstore/bash/.openapi-generator/VERSION
+++ b/samples/client/petstore/bash/.openapi-generator/VERSION
@@ -1 +1 @@
-4.2.0-SNAPSHOT
\ No newline at end of file
+4.2.1-SNAPSHOT
\ No newline at end of file
diff --git a/samples/client/petstore/bash/petstore-cli b/samples/client/petstore/bash/petstore-cli
index 17891dc77b..de7efc9b69 100755
--- a/samples/client/petstore/bash/petstore-cli
+++ b/samples/client/petstore/bash/petstore-cli
@@ -1,4 +1,5 @@
 #!/usr/bin/env bash
+
 # !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
 # !
 # ! Note:
@@ -9,6 +10,7 @@
 # !
 # !
 # !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+
 #
 # This is a Bash client for OpenAPI Petstore.
 #

There shouldn't be any Dockerfile or README changes in that client. There are similar issues with the other generators in the change list. The reason there are changes in this generator on master is because it's not included in our ensure-up-to-date script, which runs on every PR/master/branch build in CI. However, R is included in the validation script for which there should be no changes in master and there are changes in this PR.

I pulled your PR and I see that you have changes from yesterday (your master was 5 commits before previous master), which makes the changes in this PR more odd to me. Could you try the following from the repo root and see how it goes?

mvn install -am -Dmaven.javadoc.skip=true
java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar batch  \
   --includes-base-dir `pwd` --fail-fast  -- bin/ci/*

The batch command is a new one I've added recently in an attempt to speed up CI. I'm currently evaluating it before documenting and suggesting it in place of the ensure-up-to-date script.

@wing328
Copy link
Member

wing328 commented Nov 12, 2019

About bin/utils/ensure-up-to-date, it does not cover all languages/scripts under ./bin/ (intentionally) as some generators (e.g. actionscript) are barely supported. Those are being actively worked on by the community are added to ensure the samples are up-to-date and there are tests to run against the petstore samples.

@roxspring did you run ./bin/run-all-petstore to update all samples for this PR?

@roxspring
Copy link
Contributor Author

Thanks for the prompt feedback @jimschubert. Have installed elm-format and am now playing with bin/utils/ensure-up-to-date and the alternative you suggested... hopefully it'll mean I can close this MR and focus on #4454 which I was trying to work on anyway :)

@wing328
Copy link
Member

wing328 commented Nov 12, 2019

and the only thing I've found is that I was missing elm-format locally. I pinged the core team recommending that we remove language specific formatters from these scripts.

I set that up to ensure the file post-processing works as expected and also make it easier for the creator of the Elm client generator to review the change.

@roxspring
Copy link
Contributor Author

roxspring commented Nov 12, 2019

@wing328 I was running all the bin/*-petstore.sh and bin/openapi/*-petstore.sh scripts before. Hadn't come across bin/utils/ensure-up-to-date or bin/run-all-petstore - seems they run a more targeted subset. Perhaps the bin/ directory needs a dedicated README of it's own to guide noobs around!

@jimschubert the alternate command seems to work well, and quickly, once bin/ci/php-symfony-petstore.json is modified to not reference you're home directory! Once adopted, it'd be particularly nice to be able to run from a maven goal and have the whole build/test cycle from a maven one-liner.

I'll close this MR down and focus on #4454.

@roxspring roxspring closed this Nov 12, 2019
@jimschubert
Copy link
Member

@roxspring dang. Nice catch on the symfony config issue. As you can imagine, creating those JSON configs was quite a task (I ended up writing a tool to parse the bash scripts). I'll fix that one later today and check to make sure I haven't referenced my home directory elsewhere.

@jimschubert
Copy link
Member

Also, excellent suggestion on the Maven goal! I'll have to play around with it to understand how to invoke the CLI built by the current mvn build.

@jimschubert
Copy link
Member

Fixed my user path via #4470.

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.

3 participants