Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: remove validation for kpt version #8425

Conversation

ericzzzzzzz
Copy link
Contributor

@ericzzzzzzz ericzzzzzzz commented Feb 9, 2023

Fixes: #8306
Related: kptdev/kpt#3844
Related: Relevant tracking issues, for context
Merge before/after: Dependent or prerequisite PRs

Description

  • remove kpt version validation, the reason is in the linked issue.
  • upgrade kpt version in skaffold image to support parameterization, the new version kpt has already been uploaded to gcs https://storage.googleapis.com/skaffold/deps/kpt/v1.0.0-beta.24/kpt_linux_amd64
  • wget https://storage.googleapis.com/skaffold/deps/kpt/v1.0.0-beta.24/kpt_linux_amd64 -O kpt && shasum -a 256 -c ${skaffold_repo}deploy/skaffold/digests/kpt.amd64.sha256 should output ok
    Follow-up Work (remove if N/A)

@codecov
Copy link

codecov bot commented Feb 9, 2023

Codecov Report

Merging #8425 (b817f85) into main (290280e) will decrease coverage by 5.27%.
The diff coverage is 55.02%.

❗ Current head b817f85 differs from pull request most recent head 07b5996. Consider uploading reports for the commit 07b5996 to get more accurate results

@@            Coverage Diff             @@
##             main    #8425      +/-   ##
==========================================
- Coverage   70.48%   65.22%   -5.27%     
==========================================
  Files         515      607      +92     
  Lines       23150    30078    +6928     
==========================================
+ Hits        16317    19617    +3300     
- Misses       5776     8987    +3211     
- Partials     1057     1474     +417     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/completion.go 13.04% <0.00%> (-1.25%) ⬇️
cmd/skaffold/app/cmd/config/list.go 65.21% <ø> (ø)
cmd/skaffold/app/cmd/config/set.go 88.72% <ø> (ø)
cmd/skaffold/app/cmd/config/util.go 54.28% <ø> (ø)
cmd/skaffold/app/cmd/credits.go 100.00% <ø> (ø)
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/generate_pipeline.go 60.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_modules.go 65.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_profiles.go 66.66% <ø> (ø)
... and 418 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@aaron-prindle
Copy link
Contributor

  • @renzodavid9 who worked on this a bit in the past
    My understanding was that for kpt render we didn't have a max version but for kpt deploy we did. Can you confirm that kpt deploy works for all kpt versions and/or explain that for that use case there still is a version limit?

writer := kio.ByteWriter{Writer: b}
// Kpt fn render outputs Kptfile content in result, we don't want this in our manifestList as Kptfile resource cannot be deployed to k8s cluster.
pipeline := kio.Pipeline{Filters: []kio.Filter{framework.ResourceMatcherFunc(func(node *yaml.RNode) bool {
return node.GetKind() != kptfile.KptFileKind
Copy link
Contributor

@renzodavid9 renzodavid9 Feb 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I recall correctly, I think the issue is not only with the Kptfile but with all the yaml files during rendering: kpt will read all the yaml files in the folder and render them. So, in the case we have a folder structure like this:

project/
  Kptfile
  deployment.yaml
  skaffold.yaml

We'll need to filter the Kptfile and the skaffold.yaml or a deployer like kubectl will fail; I think the kpt deployer is able to filter this by itself properly. 🤔

Also, not sure how common is a folder structure like the one I mentioned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

users can also ignore skaffold.yaml file by .krmignore file, but there might be another problem related to fn render output, the command currently also output configMap file which cannot be used for deployment either, I'm gonna create an issue with kpt maintainers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't filter out skaffold.yaml ourselves, any yamls with Apiversion, Kind, metadata.name will be included regardless of which kpt version we use . It is users responsibility to make sure they put the right files in their kpt package that should include Kptfile, deployments files, data config files , we can filter deployments files from the kpt fn render. Every standard kpt Data config file is expected to have config.kubernetes.io/local-config: "true" annotation , we can inspect the annotation for each yaml resource from kpt fn render and then filter them out.

@ericzzzzzzz ericzzzzzzz force-pushed the remove-validation-for-kpt-version branch from b817f85 to 07b5996 Compare February 27, 2023 16:38
@ericzzzzzzz ericzzzzzzz merged commit 086bdc1 into GoogleContainerTools:main Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skaffold should not validate kpt version
3 participants