fix: Allow apply of manifests without file name restrictions (#6871)

[smaftoul]

Fixes: #6871

Description

We don't check anymore if the extension of files are yaml or json.

User facing changes (remove if N/A)

Before:

You could only skaffold apply file.{json,yml,yaml}

After:

Yo can skaffold apply file , with any filename but skaffold.yaml.out.

Follow-up Work

I think allowing any filename but skaffold.yaml.out is not the intended behaviour, but I don't know if it's reasonable.
Doing so is because tests creates files with this filename and not matching this filename as a kubernetes manifest allows all tests to pass.
Maybe updating testdata is a better option.
Also, there is maybe a test missing for the use case ?

[smaftoul]

Jsut signed the CLA !

[briandealwis]

Thanks @smaftoul for taking a stab at this! The underlying problem is that schema.IsSkaffoldConfig() uses the same file-name based logic. If we remove that call to HasKubernetesFileExtension() then everything should be good.

skaffold/pkg/skaffold/schema/versions.go

Lines 186 to 196 in 24180d9

	// IsSkaffoldConfig is for determining if a file is skaffold config file.
	func IsSkaffoldConfig(file string) bool {
	if !kubernetes.HasKubernetesFileExtension(file) {
	return false
	}
	if config, err := ParseConfig(file); err == nil && config != nil {
	return true
	}
	return false
	}

[smaftoul]

@briandealwis Thanks for the clever explanation !

I've removed the test on the filename and removed the extension check in IsSkaffoldConfig(), all tests are passing !

Should I squash my commit that fixes IsKubernetesManifest() into one commit ?

Now that I have fixed this the function IsKubernetesManifest looks a lot like IsSkaffoldConfig, should this be improved ? In another PR ?

Does this PR needs to add a test to show it works on non yaml / json files ?

[briandealwis]

We're good to go here: we squash on commit. Thanks for your help!

[codecov]

Codecov Report

Merging #6914 (da69f7f) into main (290280e) will decrease coverage by 1.68%.
The diff coverage is 56.96%.

@@            Coverage Diff             @@
##             main    #6914      +/-   ##
==========================================
- Coverage   70.48%   68.79%   -1.69%     
==========================================
  Files         515      554      +39     
  Lines       23150    25868    +2718     
==========================================
+ Hits        16317    17796    +1479     
- Misses       5776     6865    +1089     
- Partials     1057     1207     +150     

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/deploy.go	52.00% <ø> (-1.85%)	⬇️
cmd/skaffold/app/cmd/dev.go	84.61% <0.00%> (ø)
cmd/skaffold/app/cmd/render.go	36.66% <0.00%> (-4.72%)	⬇️
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/lsp.go	28.12% <28.12%> (ø)
cmd/skaffold/app/cmd/fix.go	68.85% <40.00%> (-7.62%)	⬇️
cmd/skaffold/app/cmd/lint.go	42.85% <42.85%> (ø)
cmd/skaffold/app/cmd/find_configs.go	48.88% <50.00%> (+0.24%)	⬆️
cmd/skaffold/app/skaffold.go	76.19% <70.00%> (-8.43%)	⬇️
... and 184 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 d4ff470...da69f7f. Read the comment docs.

[smaftoul]

@briandealwis I don't really understand why the integration tests cannot find this testdata/init/hello/deployment.yaml.
Any idea ?
make test executes fine locally, but I'm not sure it executes integration tests.

[tejal29]

@briandealwis I don't really understand why the integration tests cannot find this testdata/init/hello/deployment.yaml. Any idea ? make test executes fine locally, but I'm not sure it executes integration tests.

@smaftoul looks like the integration test is failing skaffold init --generate-manifests is failing.

Please refer to the docs here https://github.com/GoogleContainerTools/skaffold/blob/main/DEVELOPMENT.md#integration-tests to run integration tests.
Let us know if you have any issues.

[smaftoul]

$ INTEGRATION_TEST_ARGS="-run=TestInit/" make integration-in-kind  
[...]
=== RUN   TestInitManifestGeneration
=== RUN   TestInitManifestGeneration/hello
time="2021-12-17T12:28:16Z" level=info msg="Namespace: skaffoldxqgvj"
time="2021-12-17T12:28:16Z" level=info msg="Running [skaffold init -f skaffold.yaml.out --force --generate-manifests] in testdata/init/hello"
time="2021-12-17T12:28:16Z" level=info msg="Ran [skaffold init -f skaffold.yaml.out --force --generate-manifests] in 305.6102ms"
time="2021-12-17T12:28:16Z" level=info msg="Running [skaffold run --namespace skaffoldxqgvj -f skaffold.yaml.out --default-repo gcr.io/k8s-skaffold] in testdata/init/hello"
time="2021-12-17T12:28:25Z" level=info msg="Ran [skaffold run --namespace skaffoldxqgvj -f skaffold.yaml.out --default-repo gcr.io/k8s-skaffold] in 9.233 seconds"
--- PASS: TestInitManifestGeneration (9.58s)
    --- PASS: TestInitManifestGeneration/hello (9.58s)

[...]

=== Slow Tests ===
11.64s	TestInitCompose/compose
11.64s	TestInitCompose
9.58s	TestInitManifestGeneration/hello
9.58s	TestInitManifestGeneration
7.84s	TestInitKustomize/kustomize_init
7.84s	TestInitKustomize
3.89s	TestInitWithCLIArtifactAndManifestGeneration/init_with_cli_artifact_and_manifests
3.89s	TestInitWithCLIArtifactAndManifestGeneration
3.75s	TestInitWithCLIArtifact/init_with_cli_artifact
3.75s	TestInitWithCLIArtifact
0.33s	TestInitFailures/no_builder
0.33s	TestInitFailures
Deleting cluster "kind" ...

$ echo $?
0

@tejal29 I fail to reproduce the error running local integration tests in kind. Any idea what I am doing wrong ?

[SSGoku369]

kok

[MarlonGamez]

hey @smaftoul , could you try rebasing this and pushing to get these integration tests to rerun ? I'll look into this issue and see what we can do

[smaftoul]

@MarlonGamez as you can see, I rebased the PR and pushed, but the test didn't ran again. I guess the PR needs to be added the kokoro:run label.

[smaftoul]

@MarlonGamez Thanks, with git status I didn't notice I had this testdata/init/hello/deployment.yaml already existing from previous run, git status --ignored showed me it, I removed it and could reproduce the issue.

Now, that I can reproduce the issue, I have not much idea why this deployment.yaml doesn't exist. Also, I have hard time reproducing manually the issue to understand the test, as the tests removes the kind cluster it created and I cannot re-run the same commands.

Any idea of next steps to understand what is going on ?

[MarlonGamez]

hey @smaftoul , thanks for being patient with this. This failure is pretty baffling, I can't think of what's going wrong. I'll clone this and see if I can reproduce this to see what we can do

[MarlonGamez]

hey @smaftoul, sorry I haven't had much time to get to this, but I poked around the code a bit and I think I've figured out what the issue is. I can take over this PR from here and try to get it into a working state

[smaftoul]

@MarlonGamez thank you very much !
Any hint how could I have debugged that ?

[MarlonGamez]

@smaftoul I looked for code paths that were using the modified functions and found that the only other thing that could be affected was the code related to skaffold init that looked for files. I also have more context on how skaffold init works since I've done a decent amount of work on it, and unfortunately this error seems like it would be pretty hard to figure out what was happening without that context

[smaftoul]

Thanks for the explanantion 😊