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

Include vendors in this repository? #100

Closed
mpdude opened this issue Jan 22, 2020 · 6 comments
Closed

Include vendors in this repository? #100

mpdude opened this issue Jan 22, 2020 · 6 comments

Comments

@mpdude
Copy link

mpdude commented Jan 22, 2020

This action is written in JavaScript, but all the .js does is to start installing Python dependencies and to shell out to python.

That means that dependencies have to be downloaded an installed every time the action runs, which wastes bandwith, CPU, resources in general, increases the the time it takes to run. Additionally, action runs might not be repeatable.

Since GitHub Actions can also be docker containers, would you 👍 an attempt to move the relevant python stuff into a Docker container so that we could pull a finished Docker image off the Docker hub or the GitHub Package Registry?

@peter-evans
Copy link
Owner

@mpdude, thanks for raising this issue.

The Javascript/Python approach might appear strange, but there are good reasons for writing it this way. For a bit of history, this action started life during the GitHub Actions early beta when workflows were HCL and you could only write container actions that ran from a Dockerfile. When beta "v2" arrived with yaml workflows and Javascript actions I decided to migrate it to the form it's in now because container actions have a number of downsides.

  • Currently container actions are not multi-platform. You can only run them on linux virtual machines.
  • Container actions using image: 'Dockerfile' are very slow because they need to build the image from scratch every time the workflow runs.
  • Container actions using image: 'docker://my-namespace/my-image:1.0.0' cannot be forked easily because the reference to the public Docker image remains. Being able to fork GitHub actions is important for security conscious users.

So to answer your question, I'm not in favour of reverting this action back to being a container action. However, I am interested in improving its performance. You are right that downloading Python dependencies every time is not efficient. I'm planning to test a solution that would vendor the dependencies so they would not need to be downloaded each time.

@mpdude
Copy link
Author

mpdude commented Jan 23, 2020

Hey Peter, thank you for the reply!

You're right with your points against using a Docker image. In fact, the Docker image itself is not what I was after.

It's the fact that the vendors have to be downloaded on each action run, which (at least in the few observations I've made) feels a lot slower than having something "ready" from the start.

Additionally, from a security perspective, it feels a bit odd if necessary dependencies are automatically pulled at runtime. I usually have a quick scan of what an action does, but when it comes to fetching dependencies at a later time, you never know...

So, if the vendors were included (committed?) in this repo so they'd be readily available at runtime, and maybe anyone could run the "build" process themselves to see they get the same results, that would be a 💯 solution.

@mpdude mpdude changed the title Have a ready-to-go (Docker) image? Include vendors in this repository? Jan 23, 2020
@peter-evans
Copy link
Owner

I released a new version of the action with vendored Python dependencies. They can be found here. You can re-download the dependencies to test if they match the versions I've checked in to git by running the following.

(Requires npm and Python 3)

npm install
# Remove the dist dir
npm run clean
# Javascript build (ncc) and vendor Python dependencies
npm run package

You should find that everything rebuilds/downloads and there is no git diff.

Thank you for raising this issue. From a security perspective I feel much better about having the dependencies vendored.

@mpdude
Copy link
Author

mpdude commented Jan 23, 2020

Maybe we could add a short section in the README that points this out, add the commands there and then close here?

@peter-evans
Copy link
Owner

Added a section to the documentation here:
https://github.com/peter-evans/create-pull-request/blob/master/docs/concepts-guidelines.md#security

@mpdude
Copy link
Author

mpdude commented Jan 24, 2020

Great, thank you!

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

No branches or pull requests

2 participants