-
Notifications
You must be signed in to change notification settings - Fork 164
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
Dockerfiles: bump eve-alpine image, eve-cross-compilers and eve-dom0-ztools #4334
Dockerfiles: bump eve-alpine image, eve-cross-compilers and eve-dom0-ztools #4334
Conversation
df51ff3
to
0cca675
Compare
0cca675
to
527cd64
Compare
@deitch so after #4220 (comment) now bumping the installer makes problems? ;-)
|
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.
We have to automate the check on proper image hashes...
That dependency from mkverification must have moved into installer. We should check the installer to see if it is something we still actually need. |
there is https://github.com/lf-edge/eve-build-tools/blob/main/src/dockerfile-from-checker/main.go but it needs some changes |
I thought about a GH action that can report inconsistency in the hashes in different components. |
if you can run it from a GH action, then it should work, just run it with:
because there are still some deviations:
|
965e099
to
4e71198
Compare
Do we have a good reason for these deviations? |
For bpftrace-compiler I just want to use the newer alpine as we will move to it anyways, so I didn't start with the old one. For the other ones I don't know. |
} | ||
|
||
func main() { | ||
rootCmd.Flags().StringSliceP("ignore-files", "i", []string{}, "ignores specified Dockerfile; can be used several times") |
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.
Would be good to have a minimal description of what the tool does...
tools/dockerfile-from-checker/go.sum
Outdated
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 Python, it would be fewer than 100 lines and no huge go.sum
files. =(
But let it be if we already have it implemented...
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.
But can python evaluate the dockerfile variables?
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.
Ah, I got the idea... You parse AST there to get the values. Okay-okay.
4e71198
to
0f94dd8
Compare
I would suggest to make it in two different PRs: the version bump (as the original version) and new check introduction. |
ee69756
to
8595e96
Compare
Probably 4340 first, but I can rebase that one next week if this goes in. |
Let's merge this PR first, as it's already in good shape. And rebase #4334 later (it should also remove the ignoring of the |
Why are the Eden tests skipped all the time?.. @yash-zededa, any idea? |
It's because of the WF modifications. @christoph-zededa would it be possible to create a separate PR for the WF and Makefile |
@yash-zededa, do we skip the tests run if workflow files are changed? I would expect the tests to be skipped if ONLY workflows are changed... Do you think we could fix it? |
I think so, I will work on the logic where in we run tests for PR which has WF as well as code changes but ideally for all the new WF’s/modification to the existing one’s should be part of separate PR since it will help us with a clean history. |
like #4341 ? |
Merged. Rebase this one, please. |
8595e96
to
9969d1a
Compare
Makefile
Outdated
-i pkg/udev/Dockerfile \ | ||
-i pkg/xen-tools/Dockerfile \ | ||
-i pkg/xen/Dockerfile | ||
-i pkg/installer/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.
Should it be rebased to use the new variables introduced by @deitch, I guess?
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. And I just opened a PR based on that to bump the alpines as well, but remove pkg/installer
from the exceptions list 😄
Not sure if that helps, hinders or is neutral to this one. Feel free to throw mine away if needed.
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 think it's best to wait for rebase until #4348 is merged ...
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.
9969d1a
to
edf97b6
Compare
@christoph-zededa, it's time to rebase! =) #4348 is merged! |
Sync eve-alpine image across all packages with the latest image. Signed-off-by: Christoph Ostarek <[email protected]>
bsp-imx, vtpm and optee-os for cross-compilers and ztools in order to use the same image tag everywhere Signed-off-by: Christoph Ostarek <[email protected]>
edf97b6
to
742d654
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.
LGTM
Sync eve-alpine image across all packages with the latest image.