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

Remove runtime dependencies #118

Merged
merged 16 commits into from
Sep 22, 2021
Merged

Remove runtime dependencies #118

merged 16 commits into from
Sep 22, 2021

Conversation

melody-universe
Copy link
Contributor

This PR removes all runtime dependencies from the library. This allows us to execute the node script without needing a Docker image, node_modules, or a bundled asset. This was the final (probably) effort resulting from a suggestion on #42.

See a working example of this PR here.

@melody-universe melody-universe changed the title Use bundled assets Remove runtime dependencies Aug 8, 2021
@phips28
Copy link
Owner

phips28 commented Aug 8, 2021

@melody-universe this looks very promising :) I would prefer to merge this PR. As I understand it correctly, the other 2 PRs from you are obsolete if we merge this PR?
Before we can merge we should bump to 9.0.0 to avoid possible regressions. Or are you 100% sure all cases are working correctly?

@melody-universe
Copy link
Contributor Author

You are correct, the other PR's would be obsolete if we merge this PR.

I haven't tested all use cases, but I'll build some tests real quick to make sure!

@melody-universe
Copy link
Contributor Author

Just an update on this: I'm working on an automated end-to-end test suite to confirm 100% compatibility between the updated JavaScript action and the current Docker action. While the suite is expensive in terms of runtime, my hope is that it can be used for future pull requests to verify compatibility.

You can see the pull request as is working in my fork here.

It's automating commits to a test repo I've created here.

Still working on building the tests for the rest of the functionality, and right now this is testing the current version of gh-action-bump-version, but once it's done I'll merge it into the JavaScript action and hopefully we should remain at 100% passing!

At some point it might be worth considering building an integration test that proxies environment variable access as well as the spawn, console.log, and console.error functions, and I might do that in yet another PR, but the issue there is that it's testing the exact sequence of executions to various CLI's. That might be good to be 100% sure that the result of this action has not changed at all, but if we find a slightly more optimal way to call git for example, it will break that test, while passing the end-to-end test.

@joelcoxokc
Copy link

Hey can we please merge this!!!

This no longer works as it is outside the npm minor version

@joelcoxokc
Copy link

I meant to say.. Your entire github action no longer works as of this morning...
8/24/2021 because npm set their minor version. and you are still referencing node 12 in your package

@phips28
Copy link
Owner

phips28 commented Aug 24, 2021

@melody-universe whats the status on this?

@phips28
Copy link
Owner

phips28 commented Aug 24, 2021

@joelcoxokc you can set your node version in your own action.yml.
#123

@joelcoxokc
Copy link

@joelcoxokc you can set your node version in your own action.yml.
#123

Thanks @phips28 for the speedy reply and fix

@melody-universe
Copy link
Contributor Author

@phips28 Been busy with day job / life stuff. Working on getting the end-to-end testing suite up right now. I'm imagining we want to push that first and then see the test suite execute against this PR without any changes, since the end-to-end functionality should remain exactly the same.

@phips28 phips28 merged commit 08a8bff into phips28:master Sep 22, 2021
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.

3 participants