Set redeploy intent only when there are rebuilt artifacts

[gsquared94]

Fixes #5551 #4886

In an ongoing devloop the redeploy intent can be set in one of two ways: (1)there are rebuilt artifacts, or (2)the deploy dependency files have changed. In the current implementation, the redeploy intent is set as soon as there's a new request for an artifact rebuild. This doesn't work well in conjunction with the control APIs since the auto-rebuild can be off in which case the redeploy intent is processed immediately on the old image builds.

This PR queues a redeploy request of type (1) only after the artifacts have been rebuilt. This fixes the issue of the no-op deploy described in #5551 and also ensures that any rebuilt images are deployed.

[codecov]

Codecov Report

Merging #5553 (a4eca8a) into master (70cd58b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5553      +/-   ##
==========================================
- Coverage   71.08%   71.07%   -0.01%     
==========================================
  Files         402      402              
  Lines       15145    15183      +38     
==========================================
+ Hits        10766    10792      +26     
- Misses       3586     3597      +11     
- Partials      793      794       +1     

Impacted Files	Coverage Δ
pkg/skaffold/runner/changeset.go	76.47% <ø> (-1.31%)	⬇️
pkg/skaffold/runner/dev.go	73.78% <100.00%> (+1.55%)	⬆️
pkg/skaffold/schema/validation/validation.go	86.64% <0.00%> (-3.78%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70cd58b...a4eca8a. Read the comment docs.

[briandealwis]

I'm going to defer to @nkubala.

My question is: don't we need to reset the intents at line 62?

	if !needsSync && !needsBuild && !needsDeploy {
		return nil
	}

[briandealwis]

Is it possible to write some tests around this?

[gsquared94]

I'm going to defer to @nkubala.

My question is: don't we need to reset the intents at line 62?

	if !needsSync && !needsBuild && !needsDeploy {
		return nil
	}

IMO That depends on if we want to expire triggers if they aren't immediately actionable or queue them. I think @nkubala intended to have them queue up (like a single deploy request followed some time later by a single build request would run build and deploy both once). I kind of like this behavior; but agree that since it's only maintained as internal state it can cause some confusion.

Maybe a better flow would be if buildIntent == true but r.changeSet.needsRebuild == false we expire it immediately and show output Checking build status: already up-to-date and similarly for sync and deploy.

However, I don't want to change that behavior in this PR. :)

[nkubala]

I think @nkubala intended to have them queue up (like a single deploy request followed some time later by a single build request would run build and deploy both once)

this is what i intended at the time, but now i'm not so sure it's what we want. it might be a more predictable UX to just reject the intents sent by a user if skaffold isn't ready to take the corresponding action yet (e.g. receives build intent from user, but r.changeSet.needsRebuild == false

Maybe a better flow would be if buildIntent == true but r.changeSet.needsRebuild == false we expire it immediately and show output Checking build status: already up-to-date and similarly for sync and deploy.

if we decide to keep the current behavior, this is definitely something we should do. whatever route we take let's try and eliminate confusion.

cc #4886, which i think may be fixed by this issue.

[briandealwis]

I think queuing up changes would make sense if we had a general /update API, but this API makes it seem that there is explicit control over build, deploy, and sync. I found it disconcerting to see deploys happen when I asked for just "build":true.

I could see a user wanting to use "build": true to verify no build-errors, followed by a "deploy": true when they're ready to apply.