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

Action speed up by using a pre-built Docker image #107

Closed
wants to merge 15 commits into from

Conversation

vlazdra
Copy link

@vlazdra vlazdra commented May 20, 2023

Hi, came a cross this action and had no issues setting it up in my workflow. The only thing I was bummed about it the speed of the execution. So did a bit of research and came a cross this solution explained bellow.

The PR contains an action that when triggered creates a new Docker image and publishes it on GitHub Container Registry inside the active repository.
It defines a static version that is manually entered and automatically tags the latest image.
The second change is the main action itself, regarding the image that it's using. In the current example the main action uses the latest tag, but it can also use a static version depending on the definition.

Published packages look like this: My packages

And can be referenced inside the main action like so:

image: 'docker://ghcr.io/wzieba/firebase-distribution-github-action/docker-image:latest'

Note: Tested this whole implementation in my private repo.

The speed up is significant, (on average) there's a 7,5x speed increase!

Type Speed
With a pre-built Docker image ~20s
Without a pre-build Docker image ~150s

P.S. Ignore my commit naming I really lacked inspiration for names when changing one liners. 😄

@powerserg17
Copy link

Are there any considerations for this? Build time improvements seem significant. Thanks.

@vlazdra
Copy link
Author

vlazdra commented Aug 25, 2023

@wzieba Would have some time to review the changes? 🙌

@wzieba
Copy link
Owner

wzieba commented Aug 25, 2023

hi @vlazdra 👋 . Thank you for your contribution! It's a neat idea and the performance improvement is huge indeed 👍.

I wonder though: do you know any constraints regarding scalability of GitHub Container Registry? Are there any limits for fetching the image? I'm worried to not mess with users' builds (1.5k builds use this action) and this holds me back with merging this improvement.

@vlazdra
Copy link
Author

vlazdra commented Aug 25, 2023

Hey @wzieba thanks for the reply! 🙌

I've taken a look at the pricing page regarding packages and it states that it's completely free for public packages which in this case is true. Source: Pricing

"GitHub Packages usage is free for public packages."

Since I've done the PR, and left the package on my forked repo, people have started using it a little bit. My package and when I check the Usage dashboard on my profile it states zero for the usage part that counts towards the pricing limit.

What do you think?

@wzieba
Copy link
Owner

wzieba commented Aug 25, 2023

Thanks @vlazdra, this looks good 👍 . Unfortunately, I can't work on this PR now but I plan revisit this in next, next week (starting 4th Sep). Thank you for your patience and sorry for the delay!

Copy link
Owner

@wzieba wzieba left a comment

Choose a reason for hiding this comment

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

Thanks again @vlazdra for the idea and contribution! Leaving 2 comments. Let me know what you think.

Also: can I ask you to divide this PR into 2 separate PRs:

  1. One with new workflow that publishes Docker image to GitHub Registry
  2. One with the update to action.yml

? I could do this myself but wanted to preserve you as the author. Please let me know if it works for you 🙂.

@@ -38,4 +38,4 @@ branding:
icon: 'send'
runs:
using: 'docker'
image: 'Dockerfile'
image: 'docker://ghcr.io/wzieba/firebase-distribution-github-action/docker-image:latest'
Copy link
Owner

Choose a reason for hiding this comment

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

I'd vote to use the hard-coded version of the image. In case if we'll introduce breaking change between image and action's entrypoint, we'll break older builds.

Comment on lines +2 to +5
on:
push:
branches:
- master
Copy link
Owner

Choose a reason for hiding this comment

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

WDYT about trigger to be only workflow_dispatch? I think creating a new image on every push to master is not necessary. Manual trigger should be enough for this use case.

- name: Check out the repo
uses: actions/checkout@v3
- name: Build and Publish head Docker image
uses: VaultVulp/gp-docker-action@1.6.0
Copy link
Owner

Choose a reason for hiding this comment

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

Also, I'd like to suggest not using the 3rd party action but rather rely on official Docker Actions as described in this part of documentation.

I think that if there's a recommended way of publishing Docker images, VaultVulp/gp-docker-action might not be supported in the future.

Let me know if you'd like to work on that. If not, I'll take care of it 👍

@wzieba
Copy link
Owner

wzieba commented Sep 9, 2023

I've applied the suggestions in #116 and #117 and released v1.6.0 of the library.

Thanks for the contribution @vlazdra ! The speed difference is huge, great idea 🙂.

@wzieba wzieba closed this Sep 9, 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.

3 participants