-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Run tests in skaffold build, add flag to skip tests #1326
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1326 +/- ##
==========================================
+ Coverage 44.68% 44.74% +0.05%
==========================================
Files 114 114
Lines 4883 4890 +7
==========================================
+ Hits 2182 2188 +6
- Misses 2477 2478 +1
Partials 224 224
Continue to review full report at Codecov.
|
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.
A refactoring suggestion + can we write tests for this? :)
pkg/skaffold/runner/runner.go
Outdated
if err := r.Test(ctx, out, bRes); err != nil { | ||
logrus.Warnln("Skipping Deploy due to failed tests:", err) | ||
return nil | ||
if !r.opts.SkipTests { |
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.
How about pulling these out into an overriding method instead of repeating the logic?
Something like:
func (r *SkaffoldRunner) Test(ctx context.Context, out io.Writer, artifacts []build.Artifact) error {
if r.opts.SkipTests {
return nil
}
return r.Tester.Test(ctx, out, artifacts)
}
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 fact all those paths should call RunBuild()
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.
cmd/skaffold/app/cmd/cmd.go
Outdated
@@ -143,6 +143,7 @@ func AddRunDevFlags(cmd *cobra.Command) { | |||
cmd.Flags().StringArrayVarP(&opts.Profiles, "profile", "p", nil, "Activate profiles by name") | |||
cmd.Flags().StringVarP(&opts.Namespace, "namespace", "n", "", "Run deployments in the specified namespace") | |||
cmd.Flags().StringVarP(&opts.DefaultRepo, "default-repo", "d", "", "Default repository value (overrides global config)") | |||
cmd.Flags().BoolVar(&opts.SkipTests, "skip-tests", false, "whether to skip the tests after building") |
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.
s/whether/Whether
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.
fixed
pkg/skaffold/runner/runner.go
Outdated
if err := r.Test(ctx, out, bRes); err != nil { | ||
logrus.Warnln("Skipping Deploy due to failed tests:", err) | ||
return nil | ||
if !r.opts.SkipTests { |
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 fact all those paths should call RunBuild()
} | ||
|
||
if !r.opts.SkipTests { | ||
if err = r.Test(ctx, out, bRes); err != nil { |
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.
if 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.
we already have an err
defined a few lines above, so this just avoids creating another one and just overwrites the previously instantiated one
cmd/skaffold/app/cmd/build.go
Outdated
@@ -68,9 +68,9 @@ func runBuild(out io.Writer) error { | |||
buildOut = ioutil.Discard | |||
} | |||
|
|||
bRes, err := runner.Build(ctx, buildOut, runner.Tagger, config.Build.Artifacts) | |||
bRes, err := runner.RunBuild(ctx, buildOut, config.Build.Artifacts) |
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.
Could it be called BuildAndTest
?
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.
good idea
This adds support for running tests during a
skaffold build
. Additionally, it adds a way for users to skip running tests with a CLI flag.Fixes #1078