-
Notifications
You must be signed in to change notification settings - Fork 119
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
Migration to Github Actions and multiarch builds #114
Migration to Github Actions and multiarch builds #114
Conversation
.github/workflows/publish.yaml
Outdated
GIT_COMMIT=${{ github.sha }} | ||
load: true | ||
push: false | ||
tags: ghcr.io/openfaas/certifier:latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(certifier typo)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
4374bad
to
bacda06
Compare
Hi @LucasRoesler, this came up on my radar again. How does this PR now compare to the approach we took in the classic watchdog? https://github.com/openfaas/classic-watchdog/tree/master/.github/workflows |
.github/workflows/publish.yaml
Outdated
- | ||
name: Docker meta | ||
id: meta | ||
uses: crazy-max/ghaction-docker-meta@v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexellis to answer you question about how this differ from classic watchdogt is only here in how the docker metatdata is generated versus https://github.com/openfaas/classic-watchdog/blob/f5f0832493e8165695673fa6855039e08ef524a6/.github/workflows/publish.yaml#L25-L27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you see any issue in replicating the approach from the classic watchdog here, so that they are the same and easier to maintain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, originally this was my recommendation on what we should be doing. But this PR was ignored and the current pattern adopted without considering this one. I will update it to match
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks that would be appreciated. There is a rebase needed too, for the Dockerfile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexellis after making the changes i remember the main benefit of this action for creating the tag, this step understands your forked repo and creates the tag accordingly. The workflow for classic-watchdog does not handle forks correctly and can not push to the forked ghcr because the repository is fixed to openfaas
if you are ok with not having it work in forks, then i guess we can remove this, but if you want forked image builds to actually be publishable automatically, we would either need to add more complexity to the workflow or use this action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a note about using this action, it is completely templated, so the exact same snippet can be used in all repos, versus the currently hard coded repo name that needs to be updated to match the repo you copy it to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Templates make sense to me, that is something both should have IMHO.
My real concern is having two divergent builds for the two watchdogs, which should both be doing the same thing
build go binary and upload it
build docker image and upload it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they both do the same flow now, I rebased and updated to copy all of the workflows and makefile changes from classic-watchdog
but I think that crazy-max/ghaction-docker-meta@v1
is probably a more robust way to construct the docker tags and labels
**What** - Create makefile for locally building and testing - Create github workflows - Use buildx to create multiarch builds - Remove travisci Signed-off-by: Lucas Roesler <[email protected]>
bacda06
to
6ede08a
Compare
**What** - Replicate the build process from classic-watchdog. The makefile will now cross-compile the binary on the local host and the Dockerfile will simply copy the required binary instead of building it inside docker Signed-off-by: Lucas Roesler <[email protected]>
To avoid any release tags being released which have no binaries attached due to failed builds, can you take a moment to test this with your own account and to confirm it worked as expected? The suggestion you made to template the owner makes sense. If you need somewhere to copy/paste from see - https://github.com/alexellis/faasd-example/blob/master/.github/workflows/build.yml#L27 |
I don't know how to accurately test this in my own fork. Per the comment #114 (comment) I can't really test this in my own fork because the repos are hard coded to openfaas |
If I update with your copy and paste are you going to update classic-watchdog as well? |
I am not sure what you mean by "with your copy/paste"? The idea is to have two both watchdogs build and published in a consistent way. The classic watchdog could benefit from having a generated owner name, just like you suggested. It would be worth having both the same, but I don't know whether that's something I am going to pick up or another contributor. Whoever does it, needs to test that the change works on their fork before it gets merged - it looks bad when we have tags with no binaries attached because a build can't pass. |
For tracking - openfaas/classic-watchdog#3 |
Actions are enabled now, so if you can force a push, it should run. |
.github/workflows/build.yaml
Outdated
build: | ||
strategy: | ||
matrix: | ||
go-version: [ 1.13.x ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use 1.15 like the Dockerfile has?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
**What** - Calculate the repo owner from the repository owner, so that docker images can be pushed to forked GHCR Signed-off-by: Lucas Roesler <[email protected]>
0855904
to
e059b42
Compare
@alexellis here is the link to the test tag image https://github.com/users/LucasRoesler/packages/container/package/of-watchdog |
**What** - Fix copy-pasta error and use Go 1.15 for both the build and publish workflows Signed-off-by: Lucas Roesler <[email protected]>
I mean that the classic-watchdog workflow will need to be updated to allow the repo owner to be dynamic or else these two projects will have divergent GHA workflows |
FYI @techknowlogick has now updated the classic watchdog -> openfaas/classic-watchdog@681b861 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
Thanks for all the work you put into this @LucasRoesler - sorry that it took so long to get to it, and that we changed the approach a couple of times in the interim. |
This is the tag created with the latest changes, please check it and see if it looks as expected: https://github.com/openfaas/of-watchdog/releases/tag/0.8.3 I may need to make the matching Docker image public once it's available. |
Description
Motivation and Context
Relates to openfaas/faas#1585
How Has This Been Tested?
Testing in my fork
Types of changes
Checklist:
git commit -s