Skip to content

Conversation

@artheus
Copy link
Contributor

@artheus artheus commented Feb 5, 2019

  • jq executable is a much cleaner way to get version number
  • Using jq and getting releases instead of tags allows us to get latest stable release

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: master, 3.4.x, 4.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

Very simple change, simpler way to get latest release (not pre-relase) in the bash script.

* `jq` executable is a much cleaner way to get version number
* Using `jq` and getting `releases` instead of `tags` allows us to get latest stable release
@wing328
Copy link
Member

wing328 commented Feb 6, 2019

@artheus thanks for the PR.

For me, I need to install jq manually in my Mac (which is pretty easy using brew). Would it be better to check if the jq command has been installed in the machine and throw an error message accordingly?

(currently, there's no check for Python installation either but I hope you see my point providing a friendly message in case jq is not yet installed)

@wing328
Copy link
Member

wing328 commented Feb 7, 2019

cc @jimschubert

@artheus
Copy link
Contributor Author

artheus commented Feb 7, 2019

@wing328 Well, should not the change I did on row 23 do this?
I mean "This script requires 'jq' to be installed." feels like a quite ok message. No?

@wing328
Copy link
Member

wing328 commented Feb 7, 2019

@wing328 Well, should not the change I did on row 23 do this?

Right. Sorry I missed that.

I mean "This script requires 'jq' to be installed." feels like a quite ok message. No?

Looks good to me. At least the user is informed certain package needs to be installed.

@wing328 wing328 merged commit aa15882 into OpenAPITools:master Feb 7, 2019
@wing328 wing328 added this to the 4.0.0 milestone Feb 7, 2019
@jimschubert
Copy link
Member

This change forces users to install jq, which may not be an option.

For example, I'm not able to install jq on our Jenkins machines at work. The change means I'm no longer able to use this script.

Is it possible that we can use the releases change but revert back to python usage?

@jimschubert
Copy link
Member

@wing328 ^^

@wing328
Copy link
Member

wing328 commented Feb 7, 2019

@jimschubert understood but the same argument could apply to python as well (but one may argue Python is "usually" pre-installed in the build)

Ideally neither python or jq is required in the script.

What about maintaing a text file to store the latest version so that no parsing is required? (the drawback is of course the overhead to keep it up-to-date as part of the release process)

(I noticed "jq" was mentioned in the documentation before so I thought it's not a bad idea to use it)

@jimschubert
Copy link
Member

@wing328 to that argument, mvn and curl would ideally not be needed ;)

I can remove the dependency on mvn fairly easily, and this might be ideal.

Where would your proposed text file reside? My only concerns with a text file are that literally anything could exist in it, and it would be easy for an automated process to "miss" updating it. By using GitHub's API we can be confident of the response and it's structure. That's not too say I'm against a text file (it does simplify dependencies of this script).

Do you know of any other commands which might simplify down to requiring only curl and bash built-ins?

@wing328
Copy link
Member

wing328 commented Feb 7, 2019

My only concerns with a text file are that literally anything could exist in it, and it would be easy for an automated process to "miss" updating it.

We do have a release checkout script that can be updated to ensure the text filie contains the latest stable version.

Do you know of any other commands which might simplify down to requiring only curl and bash built-ins?

Not that I know of. To me, the simplest solution is to store the version in the text file.

I may have time to try to implement it tomorrow to see how easy it's

@artheus
Copy link
Contributor Author

artheus commented Feb 8, 2019 via email

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* `jq` executable is a much cleaner way to get version number
* Using `jq` and getting `releases` instead of `tags` allows us to get latest stable release
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