add skaffold build --push flag #5708
Conversation
aaron-prindle
force-pushed
the
skaff-build-push-flag
branch
2 times, most recently
from
3abc38b to
b83fd94
Compare
Codecov Report
@@ Coverage Diff @@
## master #5708 +/- ##
==========================================
+ Coverage 70.51% 70.84% +0.33%
==========================================
Files 413 421 +8
Lines 15996 16048 +52
==========================================
+ Hits 11279 11370 +91
+ Misses 3885 3845 -40
- Partials 832 833 +1
Continue to review full report at Codecov.
|
aaron-prindle
force-pushed
the
skaff-build-push-flag
branch
from
b83fd94 to
8043500
Compare
aaron-prindle
force-pushed
the
skaff-build-push-flag
branch
8 times, most recently
from
c8768fc to
431b811
Compare
|
|
||
| switch { | ||
| case runCtx.Opts.PushImages.Value() != nil: | ||
| logrus.Debugf("push value set via skaffold build --push flag, --push=%t", *runCtx.Opts.PushImages.Value()) | ||
| pushImages = *runCtx.Opts.PushImages.Value() | ||
| case pipeline.Build.LocalBuild.Push == nil: | ||
| pushImages = cl.PushImages | ||
| logrus.Debugf("push value not present, defaulting to %t because cluster.PushImages is %t", pushImages, cl.PushImages) | ||
| } else { | ||
| default: | ||
| pushImages = *pipeline.Build.LocalBuild.Push |
There was a problem hiding this comment.
Can we add tests here so we never unintentionally break the precedence?
Test to make sure if runCtx.Opts.PushImages is not set a and pipeline.Build.LocalBuild.Push is set, then pushImages is true.
If runCtx.Opts.PushImages is set to false and pipeline.Build.LocalBuild.Push is true, pushImages is set to false?
There was a problem hiding this comment.
I'll add this, thanks! Do you have any code examples of how to mock/set skaffold.yaml values or directly set the pipelines values? Still trying to understand how to do this from poking around.
There was a problem hiding this comment.
I think you can get away with just testing isImageLocal function with manually set values for runcontext.RunContext?
There was a problem hiding this comment.
Ah makes sense, added TestIsImageLocal to new_test.go testing all combinations of --push=[nil|false|true] and pipeline.Build.LocalBuild.Push=[nil|false|true] w/ desired functionality
aaron-prindle
force-pushed
the
skaff-build-push-flag
branch
from
431b811 to
96a7cef
Compare
|
Rebased using the BooleanOrUndefined type from: |
aaron-prindle
force-pushed
the
skaff-build-push-flag
branch
3 times, most recently
from
2ec1c93 to
e99f52b
Compare
aaron-prindle
force-pushed
the
skaff-build-push-flag
branch
4 times, most recently
from
dacaf2b to
ccc6d26
Compare
What is the problem being solved? Fixes GoogleContainerTools#5667, adding --push flag to skaffold build. This --push flag overrides any skaffold.yaml "build.local.push: false" configuration for a build allowing easier artifact pushing after local dev without having to modify yaml configuration. Why is this the best approach? This approach follows the style and convention for adding a flag that has been used prior for adding a flag used by a single command, similar to GoogleContainerTools#4039. Additionally this approach does not change the skaffold build default push behaviour to be false, instead only taking precendence when the --push flag is set true which is the intended use case. What other approaches did you consider? N/A What side effects will this approach have? The addition of this single flag should not have side effects on skaffold functionally outside of --push, the default behavior for skaffold build will be preserved w/o --push specified. What future work remains to be done? N/A
aaron-prindle
force-pushed
the
skaff-build-push-flag
branch
from
ccc6d26 to
10c7325
Compare
|
Rebased and added tests. |
What is the problem being solved?
Fixes #5667, adding --push flag to skaffold build. This --push flag overrides any skaffold.yaml "build.local.push: false" configuration for a build allowing easier artifact pushing after local dev without having to modify yaml configuration.
Why is this the best approach?
This approach follows the style and convention for adding a flag that has been used prior for adding a flag used by a single command, similar to #4039. Additionally this approach does not change the skaffold build default push behaviour to be false, instead only taking precendence when the --push flag is set true which is the intended use case.
What other approaches did you consider?
N/A
What side effects will this approach have?
The addition of this single flag should not have side effects on skaffold functionally outside of --push, the default behavior for skaffold build will be preserved w/o --push specified.
What future work remains to be done?
N/A