Android build fail because required parameters are not passed

[davidmfinol]

Bug description

Builds from main branch fail for Android because the required Android parameters are no longer passed

How to reproduce

• Run an Android build using game-ci/unity-builder@main
• Build errors

Expected behavior

Build should succeed

Additional details

Example failing build: https://github.com/finol-digital/Card-Game-Simulator/runs/5065670391?check_suite_focus=true#step:6:377

In particular, it seems that ANDROID_KEYSTORE_BASE64 and ANDROID_SDK_MANAGER_PARAMETERS are not going through.

EDIT: The break is from within the past week. Not sure if it was cloud runner v2 or the initial mac build support that has the regression: v2.0.0-alpha-14...main
The test workflows all passed for both of those, as we don't have a scenario for Android builds that require a custom keystore (maybe I should create a separate issue for adding this test scenario?).

[webbertakken]

At this point we should probably start thinking about extending our tests and possible restructuring some of the code to become more managable.

The index file has become a bit of a mess. We should probably use a strategy pattern for the different platforms and have each builder go through a sequence of steps that will guarantee some level of consistency between the different builder implementations.

If we have that it would be easier for contributors to plug in code to their builder implementation and add tests.

[webbertakken]

I checked AndroidKeystoreBase64 which seems to be passed through all files correctly.

In your workflow it looks like ˋnameˋ from the input class is being used https://github.com/finol-digital/Card-Game-Simulator/runs/5065670391?check_suite_focus=true#step:6:91

It says ˋname Inputˋ.

Edit: couldn't find the exact cause from my tablet.

[davidmfinol]

You can see that AndroidKeystoreBase64 is indeed received as input: Log 1

Then, during SetupCli, it is again showing OK: Log 2

But in the docker run command, it is showing as empty: Log 3

And now I see that the gap is actually on this line here:

unity-builder/src/model/image-environment-factory.ts

Lines 17 to 20 in a6ebcb5

	if (p.value.toString().includes(`\n`)) {
	string += `--env ${p.name} `;
	continue;
	}

@frostebite
Why are multi-line values are no longer passed through to docker on line 18 of image-environment-factory.ts? ANDROID_KEYSTORE_BASE64 needs to be multi-line, so can an exception be made here for ANDROID_KEYSTORE_BASE64?

[frostebite]

Not sure from reading the info attached to this ticket. Will take a deeper look.

[frostebite]

The flow of these parameters doesn't appear to have been altered significantly at all in cloud runner V2. (Although minorly in Input)

Will investigate more.

[davidmfinol]

The flow of these parameters doesn't appear to have been altered significantly at all in cloud runner V2

Right, I believe it is just lines 17-20 of image-environment-factory.ts (which was newly created in cloud runner V2) that is causing this error.
Removing those 4 lines should fix the error, but I don't know if they were added to fix some other issue in cloud runner?

[frostebite]

Thank you for the additional clarity. I will look further tomorrow and get back to you.

…

On Fri, 4 Feb 2022, 21:17 David Finol, ***@***.***> wrote: The flow of these parameters doesn't appear to have been altered significantly at all in cloud runner V2 Right, I believe it is just lines 17-20 of image-environment-factory.ts (which was newly created in cloud runner V2) that is causing this error. Removing those 4 lines should fix the error, but I don't know if they were added to fix some other issue in cloud runner? — Reply to this email directly, view it on GitHub <#334 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANLQGA6NPQ62VZE5H3WYQDUZQ7ADANCNFSM5NRVLL3A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[frostebite]

Will explore the best way to prevent this with a test case too. :) Sorry for any disruption.

…

On Sat, 5 Feb 2022, 01:50 Markus Fisher, ***@***.***> wrote: Thank you for the additional clarity. I will look further tomorrow and get back to you. On Fri, 4 Feb 2022, 21:17 David Finol, ***@***.***> wrote: > The flow of these parameters doesn't appear to have been altered > significantly at all in cloud runner V2 > > Right, I believe it is just lines 17-20 of image-environment-factory.ts > (which was newly created in cloud runner V2) that is causing this error. > Removing those 4 lines should fix the error, but I don't know if they > were added to fix some other issue in cloud runner? > > — > Reply to this email directly, view it on GitHub > <#334 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AANLQGA6NPQ62VZE5H3WYQDUZQ7ADANCNFSM5NRVLL3A> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

[webbertakken]

Was this issue resolved? Are we good to move on with releasing the next version?

[frostebite]

If this is blocking a release it can be corrected to unblock the android parameters. And I'm happy to fix for cloud runner separately. I haven't spent enough time digging into a resolution yet. Will be looking at it more in the next few days.

…

On Sat, 12 Feb 2022, 18:18 Webber Takken, ***@***.***> wrote: Was this issue resolved? Are we good to move on with releasing the next version? — Reply to this email directly, view it on GitHub <#334 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANLQGB37C6CT3ANCATG6X3U22QALANCNFSM5NRVLL3A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[davidmfinol]

I went ahead and opened a PR with the fix: #335
Tested it here: https://github.com/finol-digital/Card-Game-Simulator/runs/5177156811?check_suite_focus=true
@frostebite feel free to merge if you think it's ok

[davidmfinol]

I went ahead and merged the PR. If it breaks something in cloud runner, we can fix in another issue/PR.