fix: forceNewDeployment to be a boolean. #159

yehudacohen · 2021-01-12T22:15:14Z

*Issue #
Fixes: #157
Description of changes:
This issue was occurring due to incorrect data type of the forceNewDeployment parameter which got fixed by changing adding extra checks.

dili91 · 2021-01-23T18:00:33Z

index.js

@@ -224,7 +224,7 @@ async function run() {
    }

    const forceNewDeployInput = core.getInput('force-new-deployment', { required: false }) || false;
-    const forceNewDeployment = forceNewDeployInput != undefined && (forceNewDeployInput.toLowerCase === 'true' || forceNewDeployInput);
+    const forceNewDeployment = forceNewDeployInput != undefined && (forceNewDeployInput.toLowerCase() === 'true' || forceNewDeployInput);


To be honest I'd keep it simple and only accept boolean values here. This is what AWS SDK does and would be consistent with wait-for-service-stability param management. If a non boolean value is used, the AWS SDK validation will fail in meaningful way.
What do you think ?

Suggested change

const forceNewDeployment = forceNewDeployInput != undefined && (forceNewDeployInput.toLowerCase() === 'true' || forceNewDeployInput);

const forceNewDeployment = core.getInput('force-new-deployment', { required: false }) || false

dili91 · 2021-01-23T18:03:05Z

index.js

@@ -224,7 +224,7 @@ async function run() {
    }

    const forceNewDeployInput = core.getInput('force-new-deployment', { required: false }) || false;


Suggested change

const forceNewDeployInput = core.getInput('force-new-deployment', { required: false }) || false;

paragbhingre

@yehuda-elementryx Thank you for fixing this issue, could you please try switching the conditions like
( forceNewDeployInput || forceNewDeployInput.toLowerCase() === 'true' ) and see if the build succeeds?

dili91 · 2021-01-24T10:22:12Z

@yehuda-elementryx Thank you for fixing this issue, could you please try switching the conditions like
( forceNewDeployInput || forceNewDeployInput.toLowerCase() === 'true' ) and see if the build succeeds?

I'm neither a JS ninja nor a fan 😆 but this should work. We would accept every string values though
force-new-deployment: true => OK
force-new-deployment: "true" => OK
force-new-deployment: "whatever" => validation error from AWS SDK.

Is it really wortwhile to add that forceNewDeployInput.toLowerCase() === 'true' just to cast true strings ? tbh I'd remove any validation and manipulation on this input, and let the SDK fail if it's the case. this is what I was trying to say with my previous comment :)

That said, thanks again for looking into this @yehuda-elementryx

…tions#157

yehudacohen · 2021-01-24T17:56:41Z

Hi all, apparently I hadn't packaged the code in the previous pull request. cleaned it up slightly, fixed failing unit tests and pushed. Should be good to go now. Tested from a separate repo to ensure everything was working as expected.

yehudacohen · 2021-01-25T06:01:54Z

@yehuda-elementryx Thank you for fixing this issue, could you please try switching the conditions like
( forceNewDeployInput || forceNewDeployInput.toLowerCase() === 'true' ) and see if the build succeeds?

I'm neither a JS ninja nor a fan 😆 but this should work. We would accept every string values though
force-new-deployment: true => OK
force-new-deployment: "true" => OK
force-new-deployment: "whatever" => validation error from AWS SDK.

Is it really wortwhile to add that forceNewDeployInput.toLowerCase() === 'true' just to cast true strings ? tbh I'd remove any validation and manipulation on this input, and let the SDK fail if it's the case. this is what I was trying to say with my previous comment :)

That said, thanks again for looking into this @yehuda-elementryx

Github actions and boolean properties don't play nicely together. Take a look at this: actions/toolkit#361

paragbhingre · 2021-01-26T01:29:09Z

@yehudacohen I looked at your code and your input.test.js file also sends only string values. I'm still wondering how your code will take care of boolean values? When I tried running your code, my tests fail as I send actual boolean instead of string.

yehudacohen · 2021-01-26T02:52:21Z

@paragbhingre there are no boolean values that can be returned from the core.getInput() function. See here: actions/toolkit#361

Per that issue, all boolean values will be returned as strings.

paragbhingre · 2021-01-27T23:01:33Z

dist/index.js

@@ -388,8 +388,8 @@ async function run() {
      waitForMinutes = MAX_WAIT_MINUTES;
    }

-    const forceNewDeployInput = core.getInput('force-new-deployment', { required: false }) || false;
-    const forceNewDeployment = forceNewDeployInput != undefined && (forceNewDeployInput.toLowerCase === 'true' || forceNewDeployInput);
+    const forceNewDeployInput = core.getInput('force-new-deployment', { required: false }) || 'false';


Why do we need || 'false' condition? Can't we just have the same logic as it is done for the wait-for-service-stability parameter?

const forceNewDeployInput = core.getInput('force-new-deployment', { required: false }); const forceNewDeployment = forceNewDeployInput.toLowerCase() === 'true';

Technically it's not necessary, @paragbhingre , but I find it easier to read. This is because core.getInput('force-new-deployment', { required: false}) returns an empty string if it's not defined. To a reader who isn't familiar with the API, this makes sure they understand that if it isn't defined, the response should still be handled as a string rather than as an undefined or null value.

If you need it removed to merge the PR I'll remove it, but I find an explicit string value of 'false' cleaner than an expectation that the core.getInput() function returns an empty string when the input is not provided.

@yehudacohen Great. I guess that extra condition works for me. Thank you for fixing the issue.

paragbhingre · 2021-01-27T23:02:55Z

@paragbhingre there are no boolean values that can be returned from the core.getInput() function. See here: actions/toolkit#361

Per that issue, all boolean values will be returned as strings.

Thanks for the clarification @yehudacohen. I just have one last question that why do we have

@paragbhingre there are no boolean values that can be returned from the core.getInput() function. See here: actions/toolkit#361

Per that issue, all boolean values will be returned as strings.

Thank you for your clarification @yehudacohen, I just have one last question that I have asked in the above comment.

Bumps [eslint](https://github.com/eslint/eslint) from 8.11.0 to 8.12.0. - [Release notes](https://github.com/eslint/eslint/releases) - [Changelog](https://github.com/eslint/eslint/blob/main/CHANGELOG.md) - [Commits](eslint/eslint@v8.11.0...v8.12.0) --- updated-dependencies: - dependency-name: eslint dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

yehudacohen force-pushed the master branch from 4dd0dd3 to c4cc673 Compare January 12, 2021 22:36

allisaurus self-assigned this Jan 22, 2021

allisaurus requested a review from paragbhingre January 22, 2021 21:32

allisaurus assigned paragbhingre and unassigned allisaurus Jan 22, 2021

dili91 reviewed Jan 23, 2021

View reviewed changes

paragbhingre reviewed Jan 24, 2021

View reviewed changes

yehudacohen force-pushed the master branch 3 times, most recently from 761c9c8 to c36a5f7 Compare January 24, 2021 17:41

fix: enable forceNewDeployment for ECS Task that is broken per aws-ac…

50925f5

…tions#157

yehudacohen force-pushed the master branch from c36a5f7 to 50925f5 Compare January 24, 2021 17:50

paragbhingre reviewed Jan 27, 2021

View reviewed changes

paragbhingre approved these changes Jan 28, 2021

View reviewed changes

Merge branch 'master' into master

981433e

paragbhingre changed the title ~~Fix forceNewDeployment for ECS Task. Fixes: #157~~ fix: forceNewDeployment to be a boolean Jan 28, 2021

paragbhingre changed the title ~~fix: forceNewDeployment to be a boolean~~ fix: forceNewDeployment to be a boolean. Jan 28, 2021

mergify bot merged commit 4b6d445 into aws-actions:master Jan 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: forceNewDeployment to be a boolean. #159

fix: forceNewDeployment to be a boolean. #159

yehudacohen commented Jan 12, 2021 •

edited by paragbhingre

Loading

dili91 Jan 23, 2021 •

edited

Loading

dili91 Jan 23, 2021

paragbhingre left a comment

dili91 commented Jan 24, 2021

yehudacohen commented Jan 24, 2021

yehudacohen commented Jan 25, 2021

paragbhingre commented Jan 26, 2021

yehudacohen commented Jan 26, 2021 •

edited

Loading

paragbhingre Jan 27, 2021

yehudacohen Jan 28, 2021 •

edited

Loading

paragbhingre Jan 28, 2021

paragbhingre commented Jan 27, 2021 •

edited

Loading

	const forceNewDeployment = forceNewDeployInput != undefined && (forceNewDeployInput.toLowerCase() === 'true' \|\| forceNewDeployInput);
	const forceNewDeployment = core.getInput('force-new-deployment', { required: false }) \|\| false

		@@ -224,7 +224,7 @@ async function run() {
		}

		const forceNewDeployInput = core.getInput('force-new-deployment', { required: false }) \|\| false;

fix: forceNewDeployment to be a boolean. #159

fix: forceNewDeployment to be a boolean. #159

Conversation

yehudacohen commented Jan 12, 2021 • edited by paragbhingre Loading

dili91 Jan 23, 2021 • edited Loading

Choose a reason for hiding this comment

dili91 Jan 23, 2021

Choose a reason for hiding this comment

paragbhingre left a comment

Choose a reason for hiding this comment

dili91 commented Jan 24, 2021

yehudacohen commented Jan 24, 2021

yehudacohen commented Jan 25, 2021

paragbhingre commented Jan 26, 2021

yehudacohen commented Jan 26, 2021 • edited Loading

paragbhingre Jan 27, 2021

Choose a reason for hiding this comment

yehudacohen Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

paragbhingre Jan 28, 2021

Choose a reason for hiding this comment

paragbhingre commented Jan 27, 2021 • edited Loading

yehudacohen commented Jan 12, 2021 •

edited by paragbhingre

Loading

dili91 Jan 23, 2021 •

edited

Loading

yehudacohen commented Jan 26, 2021 •

edited

Loading

yehudacohen Jan 28, 2021 •

edited

Loading

paragbhingre commented Jan 27, 2021 •

edited

Loading