Skip to content
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

manualExit suppresses -quit, useful for buildMethods with async calls #574

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

tobyspark
Copy link
Contributor

Changes

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
Copy link

Cat Gif

@tobyspark
Copy link
Contributor Author

Notes

  • ubuntu only so far. What else would need to be done?
  • seems manualExit should be a boolean that defaults to false, but I didn’t see an example of that? Currently build.sh works on the presence of the env var, but the javascript requires a string for it to be passed through.

Copy link
Member

@webbertakken webbertakken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, mostly.

Comment on lines +34 to +37
manualExit:
required: false
default: ''
description: 'Suppresses `-quit`. Exit your build method using `EditorApplication.Exit(0)` instead.'
Copy link
Member

@webbertakken webbertakken Sep 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently you're passing the variable directly to the bash script and doing complicated logic (I believe this is the first time we use +set in this repo) that differs from other is set checks in our bash scripts.

We want to move as much away from the bash scripts as possible (in favour of the higher level TypeScript code) and not pass arbitrary values to it. Normalisation and input should either be guaranteed to be correct or throw an error so that the user knows they've misconfigured something.

Should this default to false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should default to false. Having it be the presence of a value wasn’t what I wanted, but was a first pass at using what I found; e.g. I didn’t see an example of boolean handling; Input.getInput returns a string. Happy to be pointed in the right direction.

Similarly, the bash script responds to the presence or not of the env var, which is clean from the POV of the separation of parsing the input. But I have no love for this kind of scripting and am happy to be pointed in the direction of how this this effect is achieved elsewhere in the codebase.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in a bit of a hurry atm, but basically consider workflow inputs as input for the TS logic, and the TS logic to be responsible for passing in something deterministic to the bash script.

Ideally we'd remove the string | undefined in favour of string like the rest does, and parse that into a true or false or 1 or 0 value, and pass that into the script.

@tobyspark
Copy link
Contributor Author

Now models input as a boolean. I can squash the commit if that’s desired.

Reminder – haven’t touched the other platforms.

@codecov-commenter
Copy link

Codecov Report

Merging #574 (8c6914b) into main (2190fd5) will increase coverage by 0.06%.
The diff coverage is 100.00%.

❗ Current head 8c6914b differs from pull request most recent head aab7c4f. Consider uploading reports for the commit aab7c4f to get more accurate results

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

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #574      +/-   ##
==========================================
+ Coverage   36.96%   37.02%   +0.06%     
==========================================
  Files          77       77              
  Lines        3038     3041       +3     
  Branches      641      632       -9     
==========================================
+ Hits         1123     1126       +3     
+ Misses       1915     1765     -150     
- Partials        0      150     +150     
Files Changed Coverage Δ
src/model/build-parameters.ts 89.39% <ø> (ø)
src/model/image-environment-factory.ts 7.14% <ø> (ø)
src/model/input.ts 90.98% <100.00%> (+0.22%) ⬆️

... and 36 files with indirect coverage changes

@webbertakken webbertakken merged commit a13443a into game-ci:main Sep 20, 2023
31 checks passed
@webbertakken
Copy link
Member

This'll also need to be documented in https://github.com/game-ci/documentation

Copy link
Member

@GabLeRoux GabLeRoux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

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.

4 participants