Kubectl convert - warn users with NotRegisteredError and Fail on all other errors #117002

gxwilkerson33 · 2023-03-30T01:47:07Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR solves issue of kubectl-convert failing when resources cannot be converted to desired output version. The solution is to warn the user and continue converting the rest of the file.

Which issue(s) this PR fixes:

Fixes kubernetes/kubectl#1383

Special notes for your reviewer:

Does this PR introduce a user-facing change? No

Fixed issue where kubectl-convert would fail when encountering resources that could not be converted to the specified api version. New behavior is to warn the user of the failed conversions and continue to convert the remaining resources.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2023-03-30T01:47:12Z

Please note that we're already in Test Freeze for the release-1.27 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.27.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Wed Mar 29 22:31:15 UTC 2023.

k8s-ci-robot · 2023-03-30T01:47:16Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2023-03-30T01:47:17Z

Hi @gxwilkerson33. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

gxwilkerson33 · 2023-03-30T02:04:22Z

/cc @eddiezane

eddiezane · 2023-03-30T02:08:22Z

/ok-to-test

ardaguclu · 2023-03-30T13:55:04Z

pkg/kubectl/cmd/convert/convert.go

- return nil, err
+ //Dont fail on error converting objects.
+ //Simply warn the user with the error returned from api-machinery and continue with the rest of the file
+ fmt.Fprintln(os.Stderr, err.Error())


I think, type of error matters. Perhaps we can only continue if the error is notfound error?

@ardaguclu yep. i think you are right. i will update this PR with that. Thanks

Instead of using os.Stderr directly we should plumb through the IOStreams.

…user instead of stdError

eddiezane

This is looking good but I'm not sure what the expected behavior should be.

Right now the output for multiple resources is wrapped in a v1 List.

apiVersion: v1
kind: List
metadata: {}
items:
- apiVersion: v1
  kind: Service
...

This is valid but IMO unexpected. I think users would want to pipe the output to a file with only the conversions done with a clean diff. i.e. kubectl convert -f my-old-network.yaml > my-new-network.yaml.

@ardaguclu @soltysh what do you think?

eddiezane · 2023-03-30T16:20:31Z

test/fixtures/pkg/kubectl/cmd/convert/serviceandingress.yml

+apiVersion: v1
+kind: Service
+metadata:
+ name: TestClusterIP


nit: this should be a vaild DNS-1035 value.

test-cluster-ip

eddiezane · 2023-03-30T16:20:46Z

test/fixtures/pkg/kubectl/cmd/convert/serviceandingress.yml

+apiVersion: extensions/v1beta1
+kind: Ingress
+metadata:
+ name: testIngress


nit: same as above. No capitals.

eddiezane · 2023-03-30T16:27:07Z

pkg/kubectl/cmd/convert/convert.go

@@ -263,6 +263,14 @@ func asVersionedObjects(infos []*resource.Info, specifiedOutputVersion schema.Gr

 converted, err := tryConvert(scheme.Scheme, info.Object, targetVersions...)
 if err != nil {
+ //Dont fail on not registered error converting objects.


nit: these comments should have a space.

Suggested change

//Dont fail on not registered error converting objects.

// Dont fail on not registered error converting objects.

gxwilkerson33 · 2023-03-30T20:36:32Z

@eddiezane

This is looking good but I'm not sure what the expected behavior should be.

Right now the output for multiple resources is wrapped in a v1 List.
apiVersion: v1
kind: List
metadata: {}
items:
- apiVersion: v1
  kind: Service
...
This is valid but IMO unexpected. I think users would want to pipe the output to a file with only the conversions done with a clean diff. i.e. kubectl convert -f my-old-network.yaml > my-new-network.yaml.

To clarify are you meaning not returning a list but instead returning the resources indiviually in the file?
ex:

apiVersion:v1
kind:Service
...
---
apiVersion: v1
kind:Ingress
...

Ill wait for clarification on this to push again. Thanks

ardaguclu · 2023-03-31T14:17:13Z

I think we can discard the ones not supported(we will already show a warning to user about they will be skipped), List will only contain the ones supported.

gxwilkerson33 · 2023-04-03T14:58:21Z

/retest

eddiezane

I think we can discard the ones not supported(we will already show a warning to user about they will be skipped), List will only contain the ones supported.

It looks like the current behavior is for multiple entries to be wrapped in a v1 List regardless. That's not what I would expect when using this tool but I am ok with it.

@gxwilkerson33 thanks for this PR! It will merge in a week or so after thaw.

/approve
/lgtm

k8s-ci-robot · 2023-04-04T00:15:50Z

LGTM label has been added.

Git tree hash: cb2f6df06e748464d433913ba801680110d08d2c

k8s-ci-robot · 2023-04-04T00:16:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eddiezane, gxwilkerson33

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/kubectl/OWNERS~~ [eddiezane]
~~test/fixtures/pkg/kubectl/OWNERS~~ [eddiezane]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gxwilkerson33 · 2023-04-11T23:16:24Z

/retest

gxwilkerson33 · 2023-04-12T00:04:03Z

/retest

…other errors (kubernetes#117002) * Convert file but warn user with impossible conversions * Only continuing for NotRegisteredErrors. Using iostreams for warning user instead of stdError * Formatting, correct tests to use valid DNS-1035.

Convert file but warn user with impossible conversions

420c39b

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 30, 2023

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 30, 2023

k8s-ci-robot requested a review from brianpursley March 30, 2023 01:47

k8s-ci-robot added the area/kubectl label Mar 30, 2023

k8s-ci-robot requested a review from soltysh March 30, 2023 01:47

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. area/test needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 30, 2023

gxwilkerson33 changed the title ~~Kubectl-convert-doesn't-correctly-handle-manifests-with-multiple-kinds-#1383~~ Kubectl-convert-doesn't-correctly-handle-manifests-with-multiple-kinds Mar 30, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 30, 2023

ardaguclu reviewed Mar 30, 2023

View reviewed changes

Only continuing for NotRegisteredErrors. Using iostreams for warning …

079007e

…user instead of stdError

gxwilkerson33 requested review from ardaguclu and removed request for soltysh, brianpursley and ardaguclu March 30, 2023 15:56

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 30, 2023

gxwilkerson33 changed the title ~~Kubectl-convert-doesn't-correctly-handle-manifests-with-multiple-kinds~~ Kubectl convert - warn users with NotRegisteredError and Fail on all other errors Mar 30, 2023

eddiezane reviewed Mar 30, 2023

View reviewed changes

Formatting, correct tests to use valid DNS-1035.

0a1b766

gxwilkerson33 requested a review from eddiezane April 3, 2023 15:44

eddiezane approved these changes Apr 4, 2023

View reviewed changes

k8s-ci-robot assigned eddiezane Apr 4, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 4, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 4, 2023

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 11, 2023

gxwilkerson33 closed this Apr 11, 2023

gxwilkerson33 deleted the Kubectl-convert-doesn't-correctly-handle-manifests-with-multiple-kinds-#1383 branch April 11, 2023 22:02

gxwilkerson33 restored the Kubectl-convert-doesn't-correctly-handle-manifests-with-multiple-kinds-#1383 branch April 11, 2023 22:04

gxwilkerson33 reopened this Apr 11, 2023

k8s-ci-robot merged commit 5da3867 into kubernetes:master Apr 12, 2023

k8s-ci-robot added this to the v1.28 milestone Apr 12, 2023

gxwilkerson33 deleted the Kubectl-convert-doesn't-correctly-handle-manifests-with-multiple-kinds-#1383 branch April 18, 2023 15:54

bertinatto mentioned this pull request Jun 20, 2023

ocp next openshift/kubernetes#1612

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kubectl convert - warn users with NotRegisteredError and Fail on all other errors #117002

Kubectl convert - warn users with NotRegisteredError and Fail on all other errors #117002

gxwilkerson33 commented Mar 30, 2023 •

edited

k8s-ci-robot commented Mar 30, 2023

k8s-ci-robot commented Mar 30, 2023

k8s-ci-robot commented Mar 30, 2023

gxwilkerson33 commented Mar 30, 2023 •

edited

eddiezane commented Mar 30, 2023

ardaguclu Mar 30, 2023

gxwilkerson33 Mar 30, 2023

eddiezane Mar 30, 2023

eddiezane left a comment

eddiezane Mar 30, 2023

eddiezane Mar 30, 2023

eddiezane Mar 30, 2023 •

edited

gxwilkerson33 commented Mar 30, 2023 •

edited

ardaguclu commented Mar 31, 2023

gxwilkerson33 commented Apr 3, 2023

eddiezane left a comment

k8s-ci-robot commented Apr 4, 2023

k8s-ci-robot commented Apr 4, 2023

gxwilkerson33 commented Apr 11, 2023

gxwilkerson33 commented Apr 12, 2023

	//Dont fail on not registered error converting objects.
	// Dont fail on not registered error converting objects.

Kubectl convert - warn users with NotRegisteredError and Fail on all other errors #117002

Kubectl convert - warn users with NotRegisteredError and Fail on all other errors #117002

Conversation

gxwilkerson33 commented Mar 30, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change? No

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Mar 30, 2023

k8s-ci-robot commented Mar 30, 2023

k8s-ci-robot commented Mar 30, 2023

gxwilkerson33 commented Mar 30, 2023 • edited

eddiezane commented Mar 30, 2023

ardaguclu Mar 30, 2023

Choose a reason for hiding this comment

gxwilkerson33 Mar 30, 2023

Choose a reason for hiding this comment

eddiezane Mar 30, 2023

Choose a reason for hiding this comment

eddiezane left a comment

Choose a reason for hiding this comment

eddiezane Mar 30, 2023

Choose a reason for hiding this comment

eddiezane Mar 30, 2023

Choose a reason for hiding this comment

eddiezane Mar 30, 2023 • edited

Choose a reason for hiding this comment

gxwilkerson33 commented Mar 30, 2023 • edited

ardaguclu commented Mar 31, 2023

gxwilkerson33 commented Apr 3, 2023

eddiezane left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 4, 2023

k8s-ci-robot commented Apr 4, 2023

gxwilkerson33 commented Apr 11, 2023

gxwilkerson33 commented Apr 12, 2023

gxwilkerson33 commented Mar 30, 2023 •

edited

gxwilkerson33 commented Mar 30, 2023 •

edited

eddiezane Mar 30, 2023 •

edited

gxwilkerson33 commented Mar 30, 2023 •

edited