Fix 4979 render should validate manifests

[aaron-prindle]

Fixes #4979

[aaron-prindle]

This is ready for review now (moved out of draft)

[codecov]

Codecov Report

Merging #6043 (acadbad) into master (9cd80d3) will decrease coverage by 0.00%.
The diff coverage is 58.06%.

@@            Coverage Diff             @@
##           master    #6043      +/-   ##
==========================================
- Coverage   70.20%   70.19%   -0.01%     
==========================================
  Files         475      475              
  Lines       18157    18187      +30     
==========================================
+ Hits        12747    12767      +20     
- Misses       4473     4481       +8     
- Partials      937      939       +2     

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/runner.go	57.14% <0.00%> (ø)
pkg/skaffold/schema/validation/validation.go	84.44% <58.62%> (-2.62%)	⬇️
cmd/skaffold/app/cmd/fix.go	75.55% <100.00%> (ø)
pkg/skaffold/access/provider.go	26.66% <0.00%> (-23.34%)	⬇️
pkg/skaffold/docker/parse.go	86.19% <0.00%> (-0.96%)	⬇️
pkg/skaffold/deploy/kpt/kpt.go	80.85% <0.00%> (ø)
pkg/skaffold/deploy/helm/deploy.go	78.22% <0.00%> (ø)
pkg/skaffold/deploy/kubectl/cli.go	88.09% <0.00%> (ø)
pkg/skaffold/schema/latest/v1/config.go	58.82% <0.00%> (ø)
...affold/kubernetes/portforward/forwarder_manager.go	47.91% <0.00%> (ø)
... and 3 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 9cd80d3...acadbad. Read the comment docs.

[gsquared94]

we should move these changes inside the individual Deployer.Render methods:
https://github.com/gsquared94/skaffold/blob/71ece9d2e37b11a88379c4aa97b5f0e0ec1e2b82/pkg/skaffold/deploy/kubectl/kubectl.go#L245

[tejal29]

we should move these changes inside the individual Deployer.Render methods:
https://github.com/gsquared94/skaffold/blob/71ece9d2e37b11a88379c4aa97b5f0e0ec1e2b82/pkg/skaffold/deploy/kubectl/kubectl.go#L245

Is there a reason why? I though this is validating the schema and hence should be done for every flow. "Deploy, render"

[tejal29]

Please rebase!!

[aaron-prindle]

I've rebased now. I'm currently leaving the validation as is for the config, lmk if I should modify this to a different design.

[gsquared94]

we should move these changes inside the individual Deployer.Render methods:

https://github.com/gsquared94/skaffold/blob/71ece9d2e37b11a88379c4aa97b5f0e0ec1e2b82/pkg/skaffold/deploy/kubectl/kubectl.go#L245

Is there a reason why? I though this is validating the schema and hence should be done for every flow. "Deploy, render"

We want to avoid these checks for inspect and diagnose commands. Those are (will be) frequently by the IDEs and we want to keep them fast.

[gsquared94]

I've rebased now. I'm currently leaving the validation as is for the config, lmk if I should modify this to a different design.

Putting this in parser/config.go is definitely not the right place. As you can see you're having to manage the multi config scenario yourself. Please move this to the validations package.

[gsquared94]

there are a few empty files. Should those be removed?

[aaron-prindle]

there are a few empty files. Should those be removed?

The empty files are there because the GeneratePipeline test currently violates the validation - it has manifests that do not exist. The empty files are to make the validation pass

I will move the code the validations.go and do the validation in Render, thanks Gaurav

[aaron-prindle]

Requested changes made, rebased to #6087 so that manifest values can be specified for better errors

[briandealwis]

Tests are failing.