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

Fix useDocker input parsing #10

Merged
merged 2 commits into from
Aug 31, 2022

Conversation

addianto
Copy link
Contributor

@addianto addianto commented Aug 24, 2022

Hello! First of all, I'm sorry if I did not create an issue before submitting this pull request (PR). However, since the issue and the possible fix is very straightforward, I just submitted the PR along with my explanation.

This PR fixes useDocker parsing from given boolean string (e.g. true/false string value) to a boolean value in JavaScript/TypeScript.

I noticed a problem with the parsing when trying to use this GitHub Action (v1.0.7) to deploy an app to Heroku using Git. My app does not use Dockerfile, but my deployment workflow that using this GitHub Action always attempted to deploy the app as Docker container even though I have set useDocker to false in my workflow. After I looked up the source code of this GitHub Action, I think the problem was at how useDocker is parsed.

Originally, useDocker was assigned by reading the string value from the input and parsed by instantiating a new Boolean object with string value as input for the Boolean's constructor. After that, the Boolean object is evaluated in the conditional if. The result of the evaluation always return true boolean value, hence the deployment always choose to Docker instead of Git.

I implemented a fix by replacing core.getInput() with core.getBooleanInput(). According to @actions/core documentation, core.getBooleanInput() will parse the string in the input to its correct boolean value representation. I think this approach is more intuitive than the original version where the input need to be parsed into a Boolean object constructor.

@ElayGelbart
Copy link
Owner

hey @addianto, first of all thank you for your contribution. i will merge this PR and release new version :)

@ElayGelbart ElayGelbart merged commit ab1f59e into ElayGelbart:main Aug 31, 2022
@addianto addianto deleted the bugfix/boolean-usedocker branch August 31, 2022 09:25
@addianto
Copy link
Contributor Author

Awesome. Thanks @ElayGelbart. 🙏

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.

2 participants