errorWhenMissingUnityBuildResults exposed as with parameter

[tobyspark]

Changes

Seemingly a breaking change in v2 → v3 is the action reporting error if the built player app isn’t present. Not all Unity builds are for the player, e.g. addressable content. Examining the build scripts shows a boolean errorWhenMissingUnityBuildResults managing this behaviour. This is now exposed as a with parameter in the action YAML. Also, a slightly opinionated refactor of docker.run to include an options object instead of the fixed signature requiring all options prior to errorWhen... to be defined.

Checklist

• Read the contribution guide and accept the
  code of conduct
• Docs (If new inputs or outputs have been added or changes to behavior that should be documented. Please make
  a PR in the documentation repo)
• Readme (updated or not needed)
• Tests (added, updated or not needed)

[github-actions]

[webbertakken]

Unnecessary addition to public api that doesn't add any value.

[webbertakken]

This is effectively a discrepancy in the application (builder and/or container). I feel adding an option in the (functional) public api isn't the answer to this technical issue.

We should probably agree on how the application should handle this case and then handle it correctly without exposing any new "options".

[codecov-commenter]

Codecov Report

Merging #575 (24b36fc) into main (2190fd5) will increase coverage by 0.05%.
The diff coverage is 21.42%.

❗ Current head 24b36fc differs from pull request most recent head bc748ab. Consider uploading reports for the commit bc748ab to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #575      +/-   ##
==========================================
+ Coverage   36.96%   37.01%   +0.05%     
==========================================
  Files          77       77              
  Lines        3038     3042       +4     
  Branches      641      632       -9     
==========================================
+ Hits         1123     1126       +3     
+ Misses       1915     1766     -149     
- Partials        0      150     +150     

Files Changed	Coverage Δ
src/model/build-parameters.ts	89.39% <ø> (ø)
src/model/cloud-runner/providers/docker/index.ts	16.98% <0.00%> (ø)
src/model/docker.ts	10.41% <0.00%> (-0.23%)	⬇️
src/model/input.ts	90.98% <100.00%> (+0.22%)	⬆️

... and 35 files with indirect coverage changes

[tobyspark]

This is effectively a discrepancy in the application (builder and/or container). I feel adding an option in the (functional) public api isn't the answer to this technical issue.

Is this a technical issue? There may indeed be some internal need to change this parameter depending on builder or container. But that’s not why I made the PR. There appears to be a design change in v3 that a successful action completion requires there to be a built player app. That means the Unity Builder action isn’t suitable for a whole range of uses, for example building addressables content. A quick look at the issues list shows many people hitting this, e.g. #563 #564 #567 #572

[webbertakken]

You're probably right. And you're definitely more in the know than I am.
Sorry if my review isn't accurate!

Perhaps @davidmfinol and @AndrewKahr could continue the review on this one?

[webbertakken]

Also @GabLeRoux

[tobyspark]

I’ve also just read the discord post mentioned in #563, which prompted me to re-read the code for exec-with-error-check. I’m not sure how I got “the existence of a built player app” from that, so apologies on that misdirecting inaccuracy.

The basic point remains though – that it’s requiring something specific to be output by the build method, that not all build methods will output. This makes me wonder if a more elegant design might be either

• only perform the check if a custom buildMethod is not specified in the workflow
• document a simpler string that will be checked for before trying the highly specific regex.

[webbertakken]

I think one of the problems we're facing of why we're checking for a specific text in the first place is that the exit code isn't always reliable. But it is a long time ago that I've implemented the first code for that and unfortunately I've lost context.

[tobyspark]

document a simpler string that will be checked for before trying the highly specific regex.

Separately, I’ve hit the issue that you can’t write a GITHUB_STEP_SUMMARY from one’s Unity code, as it’s running inside a container. Putting the motivation behind this PR together with that, and

• we could write out the Unity build results as a step summary from exec-with-error-check.ts
• and we’d also check for a string in stdout like GAME_CI_STEP_SUMMARY

If either pass, we don’t error.

[tobyspark]

I’m going to abandon this PR in favour of the approach above. It’s more elegant, solves more problems, and having implemented it for my own purposes, can be self-documenting for someone who experiences the conundrum of seeing ‘Build succeeded’ (i.e. exit code 0) but have the step fail on the results check.