Embed Logger inside Deployer

[nkubala]

This change embeds the Logger object's responsibilities inside of the Deployer. As "logging" can really be interpreted as "retrieving information from deployed resources", any implementation of a Logger is implicitly tied to an implementation of a Deployer in Skaffold. To pave the way for future implementations of a Deployer that are not centered around Kubernetes, we need to delegate logging responsibilities to the Deployer.

A slightly modified interface for Logger is introduced:

type Logger interface {
	StartLogger(context.Context, io.Writer, []string) error

	StopLogger()

	Mute()

	Unmute()

	SetSince(time.Time)

	RegisterBuildArtifacts([]graph.Artifact)
}

such that the logging-specific actions are more clearly identified when present on the Deployer. One additional method is added, RegisterBuildArtifacts, which allows newly-built artifacts to be added to a pre-existing Logger.

This interface is then embedded into the Deployer interface:

type Deployer interface {
	log.Logger

       ...
}

to ensure that every implementation of Deployer in Skaffold also provides an implementation for Logger.

All existing implementations of Deployer in Skaffold are Kubernetes-centric, and have been retrofitted to use the kubernetes.LogAggregator as their implementation of Logger. in the future, any new Kubernetes-centric Deployer implementations can reuse this Logger, but any new Deployer implementations that use a non-Kubernetes platform (e.g. docker) will need to implement a new Logger.

cc #5813

[gsquared94]

should we rather instantiate a LoggerProvider that can instantiate the right Logger in the corresponding Deployer? All the NewDeployer methods can take the LoggerProvider instead. This could map pretty well with a docker.NewDeployer(_, _, loggerProvider) (for the upcoming docker deployer) without any more changes to this file.

[gsquared94]

LoggerProvider can maintain a singleton instance of a KubernetesLogger.

[nkubala]

I like that idea a lot, I definitely planned on abstracting away the logging implementation from the getLogger() method, but your idea of a provider is much better than a big switch statement. I also want to move all of the get*() methods out of new.go (since this file is getting huge). WDYT about doing this in a follow up PR?

[gsquared94]

sounds good!

[gsquared94]

nit: does this conflict with the application Logger interfaces? Should we call it DeployLogger or ResourceLogger or something?

[gsquared94]

LGTM. Please address #5809 (comment) in a follow up PR.

[codecov]

Codecov Report

Merging #5809 (bcfbd89) into master (8efc9ef) will decrease coverage by 0.05%.
The diff coverage is 78.15%.

@@            Coverage Diff             @@
##           master    #5809      +/-   ##
==========================================
- Coverage   71.00%   70.95%   -0.06%     
==========================================
  Files         438      439       +1     
  Lines       16497    16521      +24     
==========================================
+ Hits        11714    11722       +8     
- Misses       3921     3935      +14     
- Partials      862      864       +2     

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/apply.go	25.00% <0.00%> (ø)
cmd/skaffold/app/cmd/delete.go	57.14% <0.00%> (ø)
cmd/skaffold/app/cmd/deploy.go	56.52% <0.00%> (ø)
cmd/skaffold/app/cmd/filter.go	25.58% <0.00%> (ø)
cmd/skaffold/app/cmd/find_configs.go	48.48% <0.00%> (ø)
cmd/skaffold/app/cmd/generate_pipeline.go	61.53% <0.00%> (ø)
cmd/skaffold/app/cmd/render.go	40.74% <0.00%> (ø)
cmd/skaffold/app/cmd/runner.go	58.49% <0.00%> (ø)
cmd/skaffold/app/cmd/test.go	46.66% <0.00%> (ø)
cmd/skaffold/app/cmd/util.go	77.77% <ø> (ø)
... and 136 more

---

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 8d05e94...bcfbd89. Read the comment docs.