Conversation
dgageot
requested review from
balopat,
nkubala,
priyawadhwa and
tejal29
as code owners
dgageot
force-pushed
the
simplify-dodev
branch
from
d7b7121 to
b22f2e2
Compare
Codecov Report
|
Signed-off-by: David Gageot <david@gageot.net>
dgageot
force-pushed
the
simplify-dodev
branch
from
b22f2e2 to
efe903b
Compare
nkubala
left a comment
nkubala
left a comment
There was a problem hiding this comment.
yeah this actually looks really good, just one comment around the case where skaffold.yaml has changed
| func (r *SkaffoldRunner) doDev(ctx context.Context, out io.Writer) error { | ||
| actionPerformed := false | ||
| if r.changeSet.needsReload { | ||
| return ErrorConfigurationChanged |
There was a problem hiding this comment.
I think we want the logs muted during this time right? otherwise we'll have deployment logs interleaved with the build/deploy logs from the new skaffold runner.
also, r.changeSet.resetReload()?
There was a problem hiding this comment.
In fact, the logs will be stopped when this function returns: here.
Also, a new runner, with a new changeset will be created.
There was a problem hiding this comment.
We should write tests expressing these expectations.
It seems that there was no log muting in the current version - but I think I agree that interleaving might happen - are you suggesting adding log muting?
There was a problem hiding this comment.
ahh very nice, thanks for clarifying. this is much cleaner! 👍
4 participants
Signed-off-by: David Gageot david@gageot.net