-
Notifications
You must be signed in to change notification settings - Fork 160
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
feat: Support for build image update when exec build or up again #570
Conversation
Signed-off-by: Qi Chen <aseaday@hotmail.com>
It is not a formal PR but for your own exp. I found some problems when check updates between image and manifest files. We could only accept this problem or we introduce a hash of the manifest and label it on the image: Hash(manifest.AST + pubkey + config_file) |
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 for your contribution! 🎉 👍
@@ -120,7 +120,50 @@ func (b generalBuilder) NumGPUs() int { | |||
return ir.NumGPUs() | |||
} | |||
|
|||
// Always return updated when met error | |||
func (b generalBuilder) CheckDepsFileUpdate(ctx context.Context, tag string, deps []string) (bool, error) { |
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.
Fancy implementation!
I am not sure how docker deals with the ImageCreateTime in our case. Did you test the logic locally?
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 test it. It seems that depends the tarball from the buildkit. So it could be cached if llb didn't change.
func (b generalBuilder) Build(ctx context.Context, pub string) error { | ||
depsFiles := []string{ | ||
pub, |
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 am not sure what pub
is, could you please explain more about 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.
The pub key is filepath "/home/aseaday/.config/envd/id_rsa_envd.pub". I think It can be changed if users bootstrap again. So it need to be added into depsFiles.
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.
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! Nice check on the dependencies!
Maybe we should document it in envd docs. |
Signed-off-by: Qi Chen aseaday@hotmail.com