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

dev: publish images to ghcr on merge to main #968

Merged
merged 14 commits into from
Feb 12, 2025
Merged

dev: publish images to ghcr on merge to main #968

merged 14 commits into from
Feb 12, 2025

Conversation

tefkah
Copy link
Member

@tefkah tefkah commented Feb 11, 2025

  • dev: push images to ghcr on main
  • dev: add same thing to on_pr as test

Issue(s) Resolved

Partially #954

High-level Explanation of PR

Pushes images to GHCR after successful merge on main.

Creates

  • pubpub/platform
  • pubpub/platform-jobs
  • pubpub/platform-migrations

and tags them with both a timestamp in :YYYYMMDDHHMMSS format and :latest.

See working log here (for on_pr, has since been removed and will only run on merge to main): https://github.com/pubpub/platform/actions/runs/13269434351/job/37045525841?pr=968

See published packages here https://github.com/orgs/pubpub/packages (they are not public yet)

I have it set to only publish these images once the deploy finishes successfully.

Feedback

During build, or after deploy

This way of doing it (pulling the image again after building, then pushing it up again) is significantly slower then just pushing it up when we are finished building the image ecrbuild-template.yml.
We could also do that, but that would have a slightly higher chance of pushing up an incorrect image (it could still fail CI/E2E/deploy).

do we think that's an okay tradeoff?

We now push the images after build is completed.

tags

I chose to do a timestamp instead of the sha we use for our normal images, bc i think it's easier to read.
We could also do the SHA, or do the SHA instead of the timestamp. any thoughts?

We now push a :${sha-short}, :${timestamp}, and :latest image.

Migrations

I think we will need the base image for migrations at some point, but i'm not really sure how we will use it yet. I've just set it up to be pushed to pubpub/platform-migrations, as that's the main use i think it will have, but if you think we'll use it for something else (???) we should maybe rename it

Test Plan

Run it baby.

Screenshots (if applicable)

Notes

@tefkah tefkah marked this pull request as ready for review February 11, 2025 18:23
Copy link
Member

@kalilsn kalilsn left a comment

Choose a reason for hiding this comment

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

Looks great! I didn't actually get the app to run but I pulled the images and they seem fine. I would change a couple things that you asked for feedback on, but none are critical so feel free to merge and we can change them as needed.

During build, or after deploy

I actually think we should do it the faster way and not worry about pushing bad images yet. Once we actually have users self hosting, I think we should explicitly tag releases that we want people to use. People using our bleeding edge latest releases can do so at their own risk!

And as far as I can tell we don't have any limit on the number of images we can store, so I'd even be interested in pushing images on PRs, and just deploying that image on merge. Relatedly, I'd love to stop running tests on main because I don't actually think it's useful when we're requiring branches be up to date before merge. ATM it's just such a long time between merging code and seeing it deployed and I would really like to shorten that.

tags

Timestamps are nice, but I'd love SHAs too. Much easier to match that up with a commit! I think the short hash would be fine.

Migrations

I don't think I know enough to have an opinion but I have a few questions:
What's the difference between the base image and core image? Base doesn't have the app compiled yet?
Is there an advantage to running the migrations with base instead of core?

@tefkah
Copy link
Member Author

tefkah commented Feb 12, 2025

Thanks for the quick review and feedback @kalilsn !

During build, after deploy

I agree with you, i'll make it happen during build. I'm not tooo sure about also doing on PR, i'm not sure we want every single build we have (which might contain secrets by accident or something) to be public. They are still available on ECR. But I don't feel too strongly about it!

RE: tests. Yeah I do think it would be nice to have faster deploys. I don't feel too strongly about keeping the e2e tests, i mostly added it to be extra double sure, but it's probably more than good enough to make sure the branch is up to date with main. If we do that, we could theoretically also skip building and pushing the images again, we could just reuse/retag the images from the last commit before merge or something. Although that might be slightly finicky, it would make deploys very quick.

Tags

Sure, I can just add the SHA one as well!

Migrations

Yeah I think our setup is kind of jank here. AFAIK the difference between core and base is that base is just the monorepo with all dependencies installed, no build. core is the built, trimmed down version of the app, ie only runtime dependencies are included, and only the ones that are actually used in files reachable from route or page files.
You can see this in the size of the images: core (pubpub-v7-core in ECR) is only 182mb, while base (pubpub-v7 in ECR) is >800mb.

I think there is value in keeping our base image small, but we would need to figure out some way to run initial seeds and migrations. This could maybe be as simple as keeping the prisma files and prisma in the core image, and automatically running migrations on startup if SELF_HOST=true or something and there are migrations to be done.
But this'll likely need to be something slightly more sophisticated, we should also include some way to backup and restore the current state of the app.

@tefkah tefkah requested a review from kalilsn February 12, 2025 13:37
@tefkah
Copy link
Member Author

tefkah commented Feb 12, 2025

Hi @kalilsn i moved the publish step earlier, at the same time that the ECR images are uploaded. You can see a successful run here (this was in the on_pr action as a test, but i removed that in the latest commit):
https://github.com/pubpub/platform/actions/runs/13286544450/job/37096435212?pr=968

As you can see it published both the :sha, the :timestamp, and the :latest tags: https://github.com/pubpub/platform/pkgs/container/platform

image

@kalilsn
Copy link
Member

kalilsn commented Feb 12, 2025

During build, after deploy

I agree with you, i'll make it happen during build. I'm not tooo sure about also doing on PR, i'm not sure we want every single build we have (which might contain secrets by accident or something) to be public. They are still available on ECR. But I don't feel too strongly about it!

I think this is great for now and am not asking you to switch it to on_pr (yet). But is there a scenario you're imagining where we would accidentally build in a secret during a PR that wouldn't also involve us exposing the secret in our git history? I'm also concerned about the possibility and think we should be cautious!

RE: tests. Yeah I do think it would be nice to have faster deploys. I don't feel too strongly about keeping the e2e tests, i mostly added it to be extra double sure, but it's probably more than good enough to make sure the branch is up to date with main. If we do that, we could theoretically also skip building and pushing the images again, we could just reuse/retag the images from the last commit before merge or something. Although that might be slightly finicky, it would make deploys very quick.

Skipping the build/push again is the real advantage I'm hoping to get since both of those are so slow. One way we could do that would be to configure ECS to pull the images right from ghcr and stop using ECR entirely.

Copy link
Member

@kalilsn kalilsn left a comment

Choose a reason for hiding this comment

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

much cleaner with the publish_to_ghcr action!

@tefkah tefkah merged commit 3a61368 into main Feb 12, 2025
6 checks passed
@tefkah tefkah deleted the tfk/ghcr-publish branch February 12, 2025 15:39
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.

2 participants