feat: respect CLI flag in skaffold render

[tejal29]

fixes #7595

In this PR,

• Add a method SetNamespace which iterates through each manifest in the manifest list.
• For every manifest, the code will
  • check if metadata section in present and set namespace to RunContext.Opts.Namespace i.e. namespace provided on CLI
  • If metadata is not present, the code will add a metadata section to the manifest and set the namespace correctly.
• The code then returns all the

PROS

• no recursion like the Visitor interface. Recursion is not required since namespace is only set in the metadata of object at top level.

CONS:

• none I could see.

Alternatives considered:

• Add a new NamespaceSetter functionality to manifestList. This functionality will go through all manifests in a manifest list and add namespace. See initial commit here 858193b#diff-589cc23b0193a874a91ea21f940516f4e17171b7e9d031175125fcbdb68e6b93R100
• This re-uses the visitor
  Cons:
• Recursively traverses through all the sections in a manifest which is not needed

TODO:

• Manually verify the behavior

[codecov]

Codecov Report

Merging #7664 (d34bfe7) into main (290280e) will decrease coverage by 3.86%.
The diff coverage is 53.59%.

@@            Coverage Diff             @@
##             main    #7664      +/-   ##
==========================================
- Coverage   70.48%   66.62%   -3.87%     
==========================================
  Files         515      587      +72     
  Lines       23150    28221    +5071     
==========================================
+ Hits        16317    18801    +2484     
- Misses       5776     8045    +2269     
- Partials     1057     1375     +318     

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/credits/export.go	0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/deploy.go	40.90% <0.00%> (-12.94%)	⬇️
cmd/skaffold/app/cmd/test.go	44.44% <0.00%> (ø)
cmd/skaffold/skaffold.go	0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/inspect_tests.go	62.50% <14.28%> (-1.14%)	⬇️
cmd/skaffold/app/cmd/render.go	35.48% <18.18%> (-5.90%)	⬇️
cmd/skaffold/app/cmd/lsp.go	28.12% <28.12%> (ø)
cmd/skaffold/app/cmd/run.go	64.28% <33.33%> (-9.63%)	⬇️
cmd/skaffold/app/cmd/fix.go	56.41% <37.50%> (-20.07%)	⬇️
cmd/skaffold/app/cmd/verify.go	41.17% <41.17%> (ø)
... and 345 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[tejal29]

Manual Verification:

on the PR branch

1. Check out the branch and make.

git checkout render_cli 
Switched to branch 'render_cli'
LOCAL=true make

2. cd integration/examples/getting-started

3. Run render

➜  getting-started git:(render_cli) ✗ ../../../out/skaffold render -d gcr.io/tejal-gke1 --namespace=test
apiVersion: v1
kind: Pod
metadata:
  name: getting-started
  namespace: test
spec:
  containers:
  - image: gcr.io/tejal-gke1/skaffold-example:v1.37.2-123-ged19a11ed
    name: getting-started

On main, namespace is not added

➜  skaffold git:(main) ✗ LOCAL=true make
GOOS=darwin GOARCH=amd64 CGO_ENABLED=1 \
	    go build -gcflags="all=-N -l" -tags "release " -ldflags "-X github.com/GoogleContainerTools/skaffold/pkg/skaffold/version.version=v1.37.2-116-gf56c65383 -X github.com/GoogleContainerTools/skaffold/pkg/skaffold/version.buildDate=2022-07-26T12:57:10Z -X github.com/GoogleContainerTools/skaffold/pkg/skaffold/version.gitCommit=f56c65383cce65d22632139fc751ee219c92e0d9 -s -w " -o out/skaffold github.com/GoogleContainerTools/skaffold/cmd/skaffold
codesign --force --deep --sign - out/skaffold
➜  skaffold git:(main) ✗ cd integration/examples/getting-started
➜  getting-started git:(main) ✗ ../../../out/skaffold render -d gcr.io/tejal-gke1 --namespace=test
apiVersion: v1
kind: Pod
metadata:
  name: getting-started
spec:
  containers:
  - image: gcr.io/tejal-gke1/skaffold-example:v1.37.2-116-gf56c65383
    name: getting-started

[aaron-prindle]

friendly ping, seems this needs to be rebased

[aaron-prindle]

nit: Is it possible/easy-fix to make the default namespace be called default vs "". Currently there seems to be some differences related to this change for rendered yaml vs how it would be done in v1, example showing namespace: "" vs namespace: default. Not sure if this will cause any issues or what guarantees we have about render being identical:

aprindle@aprindle ~/skaffold/examples/getting-started  [render_cli]$ skaffold render -a build.artifacts -o out.yaml
aprindle@aprindle ~/skaffold/examples/getting-started  [render_cli]$ cat out.yaml 
apiVersion: v1
kind: Pod
metadata:
  name: getting-started
  namespace: ""
spec:
  containers:
  - image: gcr.io/aprindle-test-cluster/skaffold-example:v1.38.0-162-g375f3c1bb@sha256:a886fa452f87d9c1d7d94669ed5b58540dd72c18b570384fabb9fcd777fe8928
    name: getting-started
aprindle@aprindle ~/skaffold/examples/getting-started  [render_cli]$ skaffold-v1.39.1 render -a build.artifacts -o out.yaml
aprindle@aprindle ~/skaffold/examples/getting-started  [render_cli]$ cat out.yaml
apiVersion: v1
kind: Pod
metadata:
  name: getting-started
  namespace: default
spec:
  containers:
  - image: gcr.io/aprindle-test-cluster/skaffold-example:v1.38.0-162-g375f3c1bb@sha256:a886fa452f87d9c1d7d94669ed5b58540dd72c18b570384fabb9fcd777fe8928
    name: getting-started

[aaron-prindle]

nit: Currently the --namespace flag reads:

  -n, --namespace='': Run deployments in the specified namespace

It might make sense to update the help text to reflect the flag works in render and modifies the manifest namespace(s) (from this change). It currently seems worded for deploy but just a nit

[tejal29]

nit: Is it possible/easy-fix to make the default namespace be called default vs "". Currently there seems to be some differences related to this change for rendered yaml vs how it would be done in v1, example showing namespace: "" vs namespace: default. Not sure if this will cause any issues or what guarantees we have about render being identical:

aprindle@aprindle ~/skaffold/examples/getting-started  [render_cli]$ skaffold render -a build.artifacts -o out.yaml
aprindle@aprindle ~/skaffold/examples/getting-started  [render_cli]$ cat out.yaml 
apiVersion: v1
kind: Pod
metadata:
  name: getting-started
  namespace: ""
spec:
  containers:
  - image: gcr.io/aprindle-test-cluster/skaffold-example:v1.38.0-162-g375f3c1bb@sha256:a886fa452f87d9c1d7d94669ed5b58540dd72c18b570384fabb9fcd777fe8928
    name: getting-started
aprindle@aprindle ~/skaffold/examples/getting-started  [render_cli]$ skaffold-v1.39.1 render -a build.artifacts -o out.yaml
aprindle@aprindle ~/skaffold/examples/getting-started  [render_cli]$ cat out.yaml
apiVersion: v1
kind: Pod
metadata:
  name: getting-started
  namespace: default
spec:
  containers:
  - image: gcr.io/aprindle-test-cluster/skaffold-example:v1.38.0-162-g375f3c1bb@sha256:a886fa452f87d9c1d7d94669ed5b58540dd72c18b570384fabb9fcd777fe8928
    name: getting-started

Done. Please check now

[tejal29]

Made another change to make sure, namespaces are not set in offline mode.
I think CD uses --offline flag.

TestRender should have same behavior in this PR and on https://github.com/GoogleContainerTools/skaffold/blob/v1.39.1/integration/render_test.go

[aaron-prindle]

LGTM!