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

bump k8s libs to v0.32.0 #3486

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

perdasilva
Copy link
Collaborator

@perdasilva perdasilva commented Jan 10, 2025

Description of the change:

Closes #3472
Closes #3470
Closes #3469
Closes #3468

Motivation for the change:

Architectural changes:

Testing remarks:

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Bug fixes are accompanied by regression test(s)
  • e2e tests and flake fixes are accompanied evidence of flake testing, e.g. executing the test 100(0) times
  • tech debt/todo is accompanied by issue link(s) in comments in the surrounding code
  • Tests are comprehensible, e.g. Ginkgo DSL is being used appropriately
  • Docs updated or added to /doc
  • Commit messages sensible and descriptive
  • Tests marked as [FLAKE] are truly flaky and have an issue
  • Code is properly formatted

Per Goncalves da Silva added 2 commits January 10, 2025 17:14
Signed-off-by: Per Goncalves da Silva <[email protected]>
Signed-off-by: Per Goncalves da Silva <[email protected]>
@perdasilva perdasilva force-pushed the bump/k8s branch 2 times, most recently from 3fac3c1 to d127432 Compare January 10, 2025 16:17
Comment on lines +28 to +30
if err := dec.Decode(unstructuredObject); err != nil {
return nil, fmt.Errorf("error decoding yaml/json object to an unstructured object: %w: %+v", err, string(data))
}
Copy link
Member

Choose a reason for hiding this comment

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

Same question for YAML/JSON/Protobuf: is it always a guarantee that the request body contains a single object? Or do we need to handle multiple objects and continuously call Decode until we see io.EOF?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The old code seems to expect a single object. I guess because it's a POST? does that assumption stand?

Copy link
Member

Choose a reason for hiding this comment

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

If old code expects a single object, we're probably safe to continue with that assumption.

Comment on lines +58 to +61
data, err := io.ReadAll(b)
if err != nil {
panic(fmt.Errorf("failed to read request body: %w", err))
}
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to pass the decoders an io.Reader rather than reading everything into memory at once?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm protobuf decoder seems to ask for data - but I can do it for that yaml

Copy link
Member

Choose a reason for hiding this comment

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

Looks like the UniversalDeserializer wants a []byte, but the YAMLOrJSON decoder accepts an io.Reader. So maybe pass readers to both, but then have decodeProtobuf be the only thing that does io.ReadAll (and YAMLOrJSON can directly read from the request body)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll pick this up on Monday. I've converted it to a draft for now. There's something else bothering me as well: 2cc85d4

need to find a nicer way to do this =S the problem is here:

opClient, err := operatorclient.NewClientFromRestConfig(validatingConfig)
func NewClientFromRestConfig(config *rest.Config) (client ClientInterface, err error) {
	kubernetes, err := kubernetes.NewForConfig(config)
	if err != nil {
		return
	}

	apiextensions, err := apiextensions.NewForConfig(config)
	if err != nil {
		return
	}

	apiregistration, err := apiregistration.NewForConfig(config)
	if err != nil {
		return
	}

	client = &Client{
		kubernetes,
		apiextensions,
		apiregistration,
	}

	return
}

Signed-off-by: Per Goncalves da Silva <[email protected]>
@perdasilva perdasilva marked this pull request as draft January 10, 2025 17:32
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants