Skip to content
This repository has been archived by the owner on Oct 13, 2021. It is now read-only.

Use dynamicCodec for decoding errors #117

Merged
merged 1 commit into from
Apr 2, 2019
Merged

Conversation

JoelSpeed
Copy link
Contributor

This fix enables proper decoding of Kubernetes API errors and converts useless messages such as error creating object: the server rejected our request for an unknown reason (post deployments.apps) into proper error messages likeerror creating object: dryRun is not supported yet

CC @samuelyallop-pusher

@JoelSpeed JoelSpeed requested a review from a team April 2, 2019 13:37
@pusher-ci
Copy link

pusher-ci commented Apr 2, 2019

@JoelSpeed: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-faros-test-1.11 90a6b28 link /test 1.11
pull-faros-test-1.12 90a6b28 link /test 1.12

Full PR test history

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. I understand the commands that are listed here.

@@ -305,7 +305,8 @@ func (a *Applier) configFor(gv schema.GroupVersion) (*rest.Config, error) {
config.APIPath = "apis/"
}

config.NegotiatedSerializer = serializer.NegotiatedSerializerWrapper(runtime.SerializerInfo{Serializer: unstructured.UnstructuredJSONScheme})
contentConfig := resource.UnstructuredPlusDefaultContentConfig()
config.NegotiatedSerializer = contentConfig.NegotiatedSerializer
Copy link

@samuelyallop-pusher samuelyallop-pusher Apr 2, 2019

Choose a reason for hiding this comment

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

Suggested change
config.NegotiatedSerializer = contentConfig.NegotiatedSerializer
config.NegotiatedSerializer = resource.UnstructuredPlusDefaultContentConfig().NegotiatedSerializer

Maybe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes the line really long so I'm tempted to skip this suggestion /shrug

@JoelSpeed
Copy link
Contributor Author

/test 1.13

Copy link
Contributor

@mthssdrbrg mthssdrbrg left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants