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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ inputs:
required: false
default: ''
description: 'Path to a Namespace.Class.StaticMethod to run to perform the build.'
manualExit:
required: false
default: ''
description: 'Suppresses `-quit`. Exit your build method using `EditorApplication.Exit(0)` instead.'
Comment on lines +34 to +37
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.

customParameters:
required: false
default: ''
Expand Down
6 changes: 6 additions & 0 deletions dist/index.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/index.js.map

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/platforms/ubuntu/steps/build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ echo ""

unity-editor \
-logfile /dev/stdout \
-quit \
$( [ "${MANUAL_EXIT}" == "true" ] || echo "-quit" ) \
-customBuildName "$BUILD_NAME" \
-projectPath "$UNITY_PROJECT_PATH" \
-buildTarget "$BUILD_TARGET" \
Expand Down
2 changes: 2 additions & 0 deletions src/model/build-parameters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class BuildParameters {
public buildFile!: string;
public buildMethod!: string;
public buildVersion!: string;
public manualExit!: boolean;
public androidVersionCode!: string;
public androidKeystoreName!: string;
public androidKeystoreBase64!: string;
Expand Down Expand Up @@ -139,6 +140,7 @@ class BuildParameters {
buildFile,
buildMethod: Input.buildMethod,
buildVersion,
manualExit: Input.manualExit,
androidVersionCode,
androidKeystoreName: Input.androidKeystoreName,
androidKeystoreBase64: Input.androidKeystoreBase64,
Expand Down
1 change: 1 addition & 0 deletions src/model/image-environment-factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class ImageEnvironmentFactory {
{ name: 'BUILD_PATH', value: parameters.buildPath },
{ name: 'BUILD_FILE', value: parameters.buildFile },
{ name: 'BUILD_METHOD', value: parameters.buildMethod },
{ name: 'MANUAL_EXIT', value: parameters.manualExit },
{ name: 'VERSION', value: parameters.buildVersion },
{ name: 'ANDROID_VERSION_CODE', value: parameters.androidVersionCode },
{ name: 'ANDROID_KEYSTORE_NAME', value: parameters.androidKeystoreName },
Expand Down
18 changes: 18 additions & 0 deletions src/model/input.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,24 @@ describe('Input', () => {
});
});

describe('manualExit', () => {
it('returns the default value', () => {
expect(Input.manualExit).toStrictEqual(false);
});

it('returns true when string true is passed', () => {
const spy = jest.spyOn(core, 'getInput').mockReturnValue('true');
expect(Input.manualExit).toStrictEqual(true);
expect(spy).toHaveBeenCalledTimes(1);
});

it('returns false when string false is passed', () => {
const spy = jest.spyOn(core, 'getInput').mockReturnValue('false');
expect(Input.manualExit).toStrictEqual(false);
expect(spy).toHaveBeenCalledTimes(1);
});
});

describe('versioningStrategy', () => {
it('returns the default value', () => {
expect(Input.versioningStrategy).toStrictEqual('Semantic');
Expand Down
6 changes: 6 additions & 0 deletions src/model/input.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,12 @@ class Input {
return Input.getInput('buildMethod') || ''; // Processed in docker file
}

static get manualExit(): boolean {
const input = Input.getInput('manualExit') || false;

return input === 'true';
}

static get customParameters(): string {
return Input.getInput('customParameters') || '';
}
Expand Down
Loading