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

Switch to javascript action instead of docker action #37

Closed
wants to merge 1 commit into from

Conversation

ejhayes
Copy link

@ejhayes ejhayes commented Jan 3, 2022

Thanks for putting this action together, very useful! This pull request improves the overall speed of this action by introducing the following changes:

@ejhayes
Copy link
Author

ejhayes commented Jan 3, 2022

@srvaroa would be great if this could be merged in, let me know if you need any additional info or changes

@srvaroa
Copy link
Owner

srvaroa commented Jul 16, 2022

Hi and apologies for the very late reply, I totally missed this in my inbox.

I don't have a strong opinion on the dockerfile vs. javascript build. However, I have near-zero knowledge of javascript so I'm afraid I would not be able to maintain the js-based build effectively.

@falbrechtskirchinger
Copy link

@srvaroa As an alternative, are you open to pre-building the Docker image?

@bennycode bennycode mentioned this pull request Nov 29, 2022
@bennycode
Copy link

@srvaroa would this PR solve #53?

@srvaroa
Copy link
Owner

srvaroa commented Dec 8, 2022

Hi @bennycode, I would definitely be happy to produce a pre-build docker image as part of the release. The part of this that I am more reluctant to merge is the switch to JavaScript as I am not able to support that effectively. I may have time to do the prebuilt docker image over the weekend but do feel free to submit a PR with that change

srvaroa added a commit that referenced this pull request Feb 14, 2023
Originally from @ejhayes at #37.

Signed-off-by: Galo Navarro <anglorvaroa@gmail.com>
srvaroa added a commit that referenced this pull request Feb 14, 2023
Originally from @ejhayes at #37.

Signed-off-by: Galo Navarro <anglorvaroa@gmail.com>
srvaroa added a commit that referenced this pull request Feb 14, 2023
Originally from @ejhayes at #37.

Signed-off-by: Galo Navarro <anglorvaroa@gmail.com>
@srvaroa
Copy link
Owner

srvaroa commented Feb 19, 2023

I have taken bits of this PR to improve the overall build time of the action to the same ballpark as the javascript build (~10s) while keeping the Dockerfile (I use a pre-built binary and download it to build the image). As I noted above my main reluctance to merging the js version is my own ability to maintain it.

Let's see if this change gives reasonable execution times. If it doesn't, I'm open to recovering this one.

Thanks @ejhayes for the PR and ideas on it. Very appreciated.

@srvaroa srvaroa closed this Feb 19, 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.

4 participants