game-ci / unity-orb

Build and test Unity projects for several platforms using CircleCI.
https://circleci.com/developer/orbs/orb/game-ci/unity
MIT License
7 stars 12 forks source link

Add parameter return-license in build job #29

Closed tadashi0713 closed 1 year ago

tadashi0713 commented 1 year ago

https://game.ci/docs/github/returning-a-license

Since Unity only allows returning professional licenses, I added a parameter to switch whether returning licenses or not for other types of licenses.

EricRibeiro commented 1 year ago

Thanks, @tadashi0713 🙇

Weird that shellcheck is complaining since you didn't touch any script. Nevertheless, is a valid error that it caught, so I'll go ahead and open a PR with the fix and then we can merge yours.

nanodeath commented 1 year ago

Speaking as a free Unity user, it'd be kinda nice if the default just worked. Can we always have this orb try to return the license, and fail gracefully and continue if it can't? Or is that a recipe for problems.

EricRibeiro commented 1 year ago

Speaking as a free Unity user, it'd be kinda nice if the default just worked. Can we always have this orb try to return the license, and fail gracefully and continue if it can't? Or is that a recipe for problems.

I don't believe it's a problem in most cases. However, It would make your job run time unnecessarily longer. The doc linked by @tadashi0713 says:

Manually returning a license is usually never necessary, unless when running into an unrecoverable error while having a license active.

Also, Unity only allows returning professional licenses.

Meaning that even for Pro and Plus, it's not always necessary to return the license. Perhaps the best approach is to set the new parameter to false as the default. What do you all think @nanodeath and @tadashi0713?

EricRibeiro commented 1 year ago

Also, @tadashi0713, can I ask you to include the new parameter in the test job? We will need it there, too, since it also uses the return-license command.

EricRibeiro commented 1 year ago

Pushed the change with my suggestions and merged the shellcheck fix, let me know what you all think.

nanodeath commented 1 year ago

Looks good to me.

tadashi0713 commented 1 year ago

@EricRibeiro Thank you for your suggestion/fix, also looks good to me 👍

KyleTryon commented 1 year ago

Looks like this resolves #26