fix: forceNewDeployment to be a boolean #140
Conversation
paragbhingre
force-pushed
the
forceNewDeploymentIssueFinal
branch
from
3457c02 to
de201af
Compare
index.js
Outdated
|
|
||
| const forceNewDeployInput = core.getInput('force-new-deployment', { required: false }) |
There was a problem hiding this comment.
this line appears to be missing a terminal ;
index.test.js
Outdated
| @@ -755,7 +755,7 @@ describe('Deploy to ECS', () => { | |||
| cluster: 'cluster-789', | |||
| service: 'service-456', | |||
| taskDefinition: 'task:def:arn', | |||
| forceNewDeployment: true | |||
| forceNewDeployment: false | |||
There was a problem hiding this comment.
the name of this test is force new deployment, but you're giving it a value of false (shouldn't this be false by default/wouldn't you want to test true?)
index.js
Outdated
|
|
||
| const forceNewDeployInput = core.getInput('force-new-deployment', { required: false }) | ||
| const forceNewDeployment = forceNewDeployInput != undefined && forceNewDeployInput.toLocaleLowerCase === "true" ? true : false; |
There was a problem hiding this comment.
nit since your conditional returns a boolean, do you need to explicitly define ? true : false?
index.js
Outdated
|
|
||
| const forceNewDeployInput = core.getInput('force-new-deployment', { required: false }) | ||
| const forceNewDeployment = forceNewDeployInput != undefined && forceNewDeployInput.toLocaleLowerCase === "true" ? true : false; |
There was a problem hiding this comment.
let's use .toLowerCase() instead, since we're not passing in a locale value
There was a problem hiding this comment.
also, nit: I'd prefer we wrap the 2 conditions of the ternary operator in parenthesis to make it clear they're being evaluated together to determine the return condition EDIT: @piradeepk 's suggestion is better
index.test.js
Outdated
| @@ -755,7 +755,7 @@ describe('Deploy to ECS', () => { | |||
| cluster: 'cluster-789', | |||
| service: 'service-456', | |||
| taskDefinition: 'task:def:arn', | |||
| forceNewDeployment: true | |||
| forceNewDeployment: false | |||
There was a problem hiding this comment.
this condition is not correct - this test is meant to test the success of passing in forceNewDeployment, so we would expect ecs:update-service to be called with this param set as true. If this is not occurring, either the setup of the test is wrong or the change is not behaving as we expect.
index.js
Outdated
|
|
||
| const forceNewDeployInput = core.getInput('force-new-deployment', { required: false }) | ||
| const forceNewDeployment = forceNewDeployInput != undefined && forceNewDeployInput.toLocaleLowerCase === "true" ? true : false; |
There was a problem hiding this comment.
also, nit: I'd prefer we wrap the 2 conditions of the ternary operator in parenthesis to make it clear they're being evaluated together to determine the return condition EDIT: @piradeepk 's suggestion is better
|
|
||
| const forceNewDeployInput = core.getInput('force-new-deployment', { required: false }); | ||
| const forceNewDeployment = forceNewDeployInput != undefined && (forceNewDeployInput.toLowerCase === 'true' || forceNewDeployInput); |
There was a problem hiding this comment.
Curious why you're adding the || forceNewDeployInput as a condition here?
There was a problem hiding this comment.
forceNewDeployInput was coming as a string which was the main issue that we solved with the following condition forceNewDeployInput != undefined && forceNewDeployInput.toLowerCase === 'true' but the test case sends an actual boolean value which was not taken care by the above condition.
Now, we have added extra condition of || forceNewDeployInput that takes care of the boolean value of the forceNewDeploy as well.
How does that sound to you @piradeepk ?
There was a problem hiding this comment.
Ship it! 💯 Thanks for fixing this so quickly @paragbhingre!
|
hi, thank you for this fix! @piradeepk When can we expect a release with this revision? Thank you! |
Bumps [jest](https://github.com/facebook/jest) from 27.4.2 to 27.4.3. - [Release notes](https://github.com/facebook/jest/releases) - [Changelog](https://github.com/facebook/jest/blob/main/CHANGELOG.md) - [Commits](jestjs/jest@v27.4.2...v27.4.3) --- updated-dependencies: - dependency-name: jest dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This PR fix the Issue #133
Description of changes:
This issue was occurring due to incorrect data type of the forceNewDeployment parameter.
In order to fix this issue need to type caste the value of the forceNewDeployment parameter from string to boolean.