Track deployment namespaces in Deployer

[nkubala]

Related: #6117

Description

Currently, the Deploy() method defined on all Deployer implementations returns two values: any error seen during the deployment, and a list of namespaces deployed to. This list of namespaces is then returned back to the Runner, which updates them on the RunContext to be used later. This pattern is:

1. Unnecessary, because all of the components that use runContext.GetNamespaces() live inside the Deployer implementation, and
2. Problematic, because it defines a messy API for future Deployer implementations that have no use for the concept of a namespace

This change moves the tracking of all deployed namespaces into the Deployer, which can then feed those namespaces to any subcomponents that require them to function properly.

This changes the Deploy call to a very simple API:

Deploy(context.Context, io.Writer, []graph.Artifact) error

Additionally, all sub-components that provide a Start() method for orchestration by the Runner (Accessor, Debugger, Monitor, and Logger have had the namespaces parameter removed from their signatures. Using Accessor as an example:

type Accessor interface {
  Start(context.Context, io.Writer)
}

All Kubernetes-based Deployer implementations provide a trackNamespaces() method, called at the end of a Deploy(), which will update an internal []string with the new namespaces. This []string is then passed by reference to all subcomponents, which can consume whatever the contents are as the effective namespaces.

NOTE: We can safely use a *[]string to represent the active namespaces, as we know that no subcomponents will be activated by the Runner until the Deploy() call exits successfully.

This change is a non-functional refactor.

[codecov]

Codecov Report

Merging #6170 (b6debf8) into master (634ad08) will increase coverage by 0.06%.
The diff coverage is 68.53%.

@@            Coverage Diff             @@
##           master    #6170      +/-   ##
==========================================
+ Coverage   71.20%   71.27%   +0.06%     
==========================================
  Files         481      479       -2     
  Lines       21587    21550      -37     
==========================================
- Hits        15371    15359      -12     
+ Misses       5235     5212      -23     
+ Partials      981      979       -2     

Impacted Files	Coverage Δ
pkg/skaffold/access/access_mux.go	0.00% <0.00%> (ø)
pkg/skaffold/debug/debugger_mux.go	0.00% <0.00%> (ø)
pkg/skaffold/log/logger_mux.go	0.00% <0.00%> (ø)
pkg/skaffold/runner/notification.go	0.00% <0.00%> (ø)
pkg/skaffold/runner/runcontext/context.go	90.07% <ø> (+0.52%)	⬆️
pkg/skaffold/runner/v1/apply.go	0.00% <0.00%> (ø)
pkg/skaffold/runner/v1/dev.go	73.12% <0.00%> (ø)
pkg/skaffold/sync/sync.go	73.21% <0.00%> (ø)
pkg/skaffold/runner/v1/deploy.go	76.47% <33.33%> (-0.35%)	⬇️
pkg/skaffold/deploy/deploy_mux.go	45.45% <50.00%> (-3.30%)	⬇️
... and 15 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 634ad08...b6debf8. Read the comment docs.

[tejal29]

Can we get rid of all the Provider interfaces before this. That would help clean up few interfaces.

[MarlonGamez]

couple small nits, nothing major

[nkubala]

Can we get rid of all the Provider interfaces before this. That would help clean up few interfaces.

@tejal29 should that block this change? not saying we shouldn't clean it up, but I'm not convinced it needs to happen before this, it seems a bit orthogonal.

[tejal29]

All Kubernetes-based Deployer implementations provide a trackNamespaces() method, called at the end of a Deploy(), which will update an internal []string with the new namespaces. This []string is then passed by reference to all subcomponents, which can consume whatever the contents are as the effective namespaces.

Is this namespace now part of Deploy Object. Can we remove it from RunContext?
Also a bunch of methods in PortForwarder, Logger and Debugger have changed right?
Can you list them in the description

[tejal29]

Can we get rid of all the Provider interfaces before this. That would help clean up few interfaces.

@tejal29 should that block this change? not saying we shouldn't clean it up, but I'm not convinced it needs to happen before this, it seems a bit orthogonal.

The number of unnecessary changes in this PR will reduce. If we do them later, we will be adding a bunch of changes and then deleting them in the later PR.

[nkubala]

@tejal29

Is this namespace now part of Deploy Object. Can we remove it from RunContext?

Good catch, meant to do this. Updated

Also a bunch of methods in PortForwarder, Logger and Debugger have changed right? Can you list them in the description?

Done.

#6190 has also been merged to remove the Provider interfaces, which should make this PR easier to review.

addressed all requested changes

[tejal29]

LGMT.

Testing deploy now.

[tejal29]

yay!

[tejal29]

Testing Notes

1. trackNamespaces is called in all 4 deployers.
   

2. Debug works for cloudcode-samples/nodejs/ in default namespace.

• debugging break point hit.
• portfowarded urls working.

3. Debug works for cloudcode-samples/nodejs/ in a test namespace using SKAFFOLD_NAMESPACE environment variable in debug config.

• debugging break point hit.
• portfowarded urls working.

4. Testing helm deplyer

➜  helm-deployment git:(deploy-namespaces) ✗ skaffold dev --namespace test
 - test:deployment/skaffold-helm is ready.
Deployments stabilized in 2.235 seconds
Press Ctrl+C to exit
Watching for changes...
[skaffold-helm] Hello world! 0
[skaffold-helm] Hello world! 1
[skaffold-helm] Hello world! 0
[skaffold-helm] Hello world! 1
[skaffold-helm] Hello world! 2
[skaffold-helm] Hello world! 2

[gsquared94]

a bad merge seems to have reverted #6135