-
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
More tests #2128
More tests #2128
Conversation
4d4bdda
to
b190fbf
Compare
Codecov Report
@@ Coverage Diff @@
## master #2128 +/- ##
========================================
+ Coverage 56.79% 57.3% +0.5%
========================================
Files 183 182 -1
Lines 7890 7869 -21
========================================
+ Hits 4481 4509 +28
+ Misses 2995 2948 -47
+ Partials 414 412 -2
Continue to review full report at Codecov.
|
return append(s[:i], s[i+1:]...) | ||
for i := len(s) - 1; i >= 0; i-- { | ||
if s[i] == target { | ||
s = append(s[:i], s[i+1:]...) |
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.
😳 was this causing bugs anywhere?
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 don’t think so. I added tests and realized it was doing the job only partially
@@ -91,8 +91,7 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*la | |||
// Watch artifacts | |||
for i := range artifacts { | |||
artifact := artifacts[i] | |||
|
|||
if !r.IsTargetImage(artifact) { | |||
if !r.runCtx.Opts.IsTargetImage(artifact) { |
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 was broken before then?
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.
No. I just moved the function
This needs a rebase :) |
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
It’s now easier to understand what the pod spec looks like. Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
@nkubala @priyawadhwa It has been rebased |
No description provided.