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

feat: Switching the action from docker into composite #42

Merged
merged 2 commits into from
Jan 17, 2023

Conversation

crisperit
Copy link

@crisperit crisperit commented Dec 25, 2022

@jsmrcaga Here is the another approach - to use already installed netlify-cli

The time difference is pretty the same as with the prepared specially docker image - but it might be easier to maintain that's why I like that version more

I've tested that action only with a scenario of not having to run nvm, install step or build step

@jsmrcaga You can try to test it more

Fixes #35

Copy link
Owner

@jsmrcaga jsmrcaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @crisperit , I'm so sorry this is taking so long, I'm caught up in different projects.
Your PR looks good! I was thinking that it was not necessary to completely remove the entrypoint.sh file though, you could just leave it as is and call it via the composite action if I'm not mistaken. It would make for a way simpler action.yml file and much less diff in the pull request ;)
it would also allow to make the .sh file "testable" if we ever decide/need to add unit testing

action.yml Outdated
Comment on lines 62 to 77
- name: "Install node through NVM if needed"
shell: bash
run: |
if [[ "${{ inputs.use_nvm }}" == "true" ]] && ([[ -n "${{ inputs.node_version }}" ]] || [[ -e ".nvmrc" ]])
then
curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.35.2/install.sh | bash
[ -s "$HOME/.nvm/nvm.sh" ] && \. "$HOME/.nvm/nvm.sh"
if [[ -n "${{ inputs.node_version }}" ]]
then
nvm install "${{ inputs.node_version }}"
else
nvm install
fi
else
echo "Node installation has been omitted"
fi
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not necessary anymore 😉
since we have a composite action, users are free to use actions/setup-node to setup whatever version they need, and our action should run with that version.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok - removed

@crisperit crisperit force-pushed the using-composite-instead-of-docker branch from c5522c3 to 25f9410 Compare January 12, 2023 07:05
@crisperit
Copy link
Author

Hi @crisperit , I'm so sorry this is taking so long, I'm caught up in different projects. Your PR looks good! I was thinking that it was not necessary to completely remove the entrypoint.sh file though, you could just leave it as is and call it via the composite action if I'm not mistaken. It would make for a way simpler action.yml file and much less diff in the pull request ;) it would also allow to make the .sh file "testable" if we ever decide/need to add unit testing

Done - I've returned entrypoint.sh

@crisperit crisperit requested a review from jsmrcaga January 12, 2023 07:18
@jsmrcaga
Copy link
Owner

@crisperit looks amazing! Sorry again for the delay!
There is some auto-formatting but since there are so many changes anyway it's ok 😉

I'm gonna go ahead and release a major version, thanks a lot for all the work you put in! If you'd like you can add your name to the README as well in another PR

@jsmrcaga jsmrcaga merged commit 10294b8 into jsmrcaga:master Jan 17, 2023
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.

Netlify is pre-installed on ubuntu-latest
3 participants