-
Notifications
You must be signed in to change notification settings - Fork 28
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: push final image to cache repo #197
Changes from all commits
d53e46a
0765f69
f416b7b
3a99f94
d008cc0
e6d7b1d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -477,14 +477,19 @@ func Run(ctx context.Context, options Options) error { | |
if val, ok := os.LookupEnv("KANIKO_REGISTRY_MIRROR"); ok { | ||
registryMirror = strings.Split(val, ";") | ||
} | ||
image, err := executor.DoBuild(&config.KanikoOptions{ | ||
var destinations []string | ||
if options.CacheRepo != "" { | ||
destinations = append(destinations, options.CacheRepo) | ||
} | ||
opts := &config.KanikoOptions{ | ||
// Boilerplate! | ||
CustomPlatform: platforms.Format(platforms.Normalize(platforms.DefaultSpec())), | ||
SnapshotMode: "redo", | ||
RunV2: true, | ||
RunStdout: stdoutWriter, | ||
RunStderr: stderrWriter, | ||
Destinations: []string{"local"}, | ||
Destinations: destinations, | ||
NoPush: len(destinations) == 0, | ||
CacheRunLayers: true, | ||
CacheCopyLayers: true, | ||
CompressedCaching: true, | ||
|
@@ -515,11 +520,16 @@ func Run(ctx context.Context, options Options) error { | |
RegistryMirrors: registryMirror, | ||
}, | ||
SrcContext: buildParams.BuildContext, | ||
}) | ||
} | ||
image, err := executor.DoBuild(opts) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to see more debugging/trace logging around this step and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what the ask is here, I didn't change any of this code so seems out of scope? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't you add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I just moved opts to a variable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I missed L483. Anyway, I wanted to improve logging around this, but if you think this is useless here, I'm cool with leaving it as is. |
||
if err != nil { | ||
return nil, err | ||
return nil, xerrors.Errorf("do build: %w", err) | ||
} | ||
if err := executor.DoPush(image, opts); err != nil { | ||
return nil, xerrors.Errorf("do push: %w", err) | ||
} | ||
endStage("🏗️ Built image!") | ||
|
||
return image, err | ||
} | ||
|
||
|
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.
This causes problems when
DoPush
is called. I'm not sure what the original intent was.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 you elaborate on what the problems are? Are you referring to #185 (comment)?
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'm pretty sure this was simply a bug that was present because
DoPush
wasn't being used previously. Kaniko expects you to either set it or use--no-push
.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 we make it optional too? If platform engineers are concerned about envbuilder speed, they can
--enable-do-push
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.
It depends, do we want to expose multiple knobs, one for caching and one for pushing final image/build optimization, or do we want these to be behind one knob? I'm open to splitting it up but could use some thought. Just because it's two different moments in Kaniko, doesn't mean we have to follow that here.