-
Notifications
You must be signed in to change notification settings - Fork 0
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
expose build args, format file, update deps and fix diggest upload #44
Conversation
e4ee401
to
e832e69
Compare
c6d1e39
to
57c1655
Compare
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.
Added a couple comments, lgtm in general though.
.github/workflows/build.yaml
Outdated
with: | ||
aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }} | ||
aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }} | ||
aws-region: ${{ inputs.region }} | ||
|
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.
nit: I add the newlines for a visual separation between steps :)
- name: Upload digest | ||
uses: actions/upload-artifact@v4 | ||
with: | ||
name: digests-${{ env.PLATFORM_DASH_PAIR }} | ||
name: digests-${{ inputs.docker_target }}-${{ env.IMAGE_DIGEST }} |
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.
The docker target isn't a required argument- what happens if it's not set? Just digests--<digest>
? I think the platform is pretty important to include- I'm using depending on it as digests-{{ matrix.platform }}-*
in the download steps elsewhere so it can properly attribute the arch. I'd suggest adding something instead of replacing the platform env?
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.
in this case it will generate a file like digest--2369058236gjgfhsdjgsjdtgyu23u852365
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.
let me update the problem in the read me, when using the current strategy twice (parliament and owl) it fails because the file already exists.
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.
Yup, that would totally happen! The IMAGE_DIGEST should stop that for sure.
859f9a6
to
1738166
Compare
this commit updates the yaml file, updating github action versions of the dependencies, exposing the GOARCH, BPF_ARCH, and GOOS environment variables in the docker build with build args, and fix the diggest upload filename that was failing when this action was called more than once. Signed-off-by: Sebastian Webber <[email protected]>
1738166
to
13ca8ec
Compare
when using the build workflow for multi-arch images multiple times, it fails because the file already exists.
Related: https://github.com/timescale/savannah-owl/pull/346