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

Add support for passing the fieldValidation query parameter on patch #929

Merged
merged 7 commits into from
Jun 13, 2022

Conversation

phroggyy
Copy link
Contributor

@phroggyy phroggyy commented Jun 7, 2022

The fieldValidation flag should be set to Ignore when doing a server-side
apply for a partial object. This introduces support to configure the
fieldValidation parameter to all the available options.

Motivation

In order to increase parity with the Go library, and to support partial (invalid, but applicable for patch) server-side Patch::Apply requests, the kube library should support a way to configure the fieldValidation query parameter.

Solution

This MR introduces a field_validation property and method on PatchParams, allowing the configuration of the fieldValidation query parameter.

@phroggyy phroggyy force-pushed the feat/add-field-validation-directive branch 2 times, most recently from 1840bf9 to 6987d6d Compare June 7, 2022 21:28
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this. Sensible tests. It looks sensible to me some minor comments only on documentation.

kube-core/src/params.rs Show resolved Hide resolved
kube-core/src/params.rs Show resolved Hide resolved
@phroggyy
Copy link
Contributor Author

phroggyy commented Jun 7, 2022

Tests failing and it seems to be because kube-rs is misusing the form_urlencoded crate. The Serializer takes a list of parameters to construct the params.

However, kube-rs abuses this in the way it constructs the patch URL. It uses the path as a prefix, while the serialiser is exclusively intended for the construction of a tuple-based urlencoded string. This could be worked around by setting the start offset properly, but that is out of scope for this MR.

Therefore, for now, I'm updating the test to knowingly assert a weird string ?&key=val. Will log a follow-up ticket to add some tests for the requests being made, and somehow correct the usage of the serialiser.

kube-core/src/params.rs Outdated Show resolved Hide resolved
kube-core/src/params.rs Show resolved Hide resolved
kube-core/src/params.rs Show resolved Hide resolved
@clux
Copy link
Member

clux commented Jun 7, 2022

Tests failing and it seems to be because kube-rs is misusing the form_urlencoded crate. The Serializer takes a list of parameters to construct the params.

However, kube-rs abuses this in the way it constructs the patch URL. It uses the path as a prefix, while the serialiser is exclusively intended for the construction of a tuple-based urlencoded string. This could be worked around by setting the start offset properly, but that is out of scope for this MR.

Therefore, for now, I'm updating the test to knowingly assert a weird string ?&key=val. Will log a follow-up ticket to add some tests for the requests being made, and somehow correct the usage of the serialiser.

Ah, I think some of this might have been made necessary by other prefixes being allowed to sneak in via cluster_url in the kubeconfig, but maybe there is a more elegant way. Happy to see changes if we are misusing things.

Generally when we have been testing this we have been testing at the Request level (i.e. in kube-core/src/request.rs) and asserting the req.uri() from that. But your substring suggestion is probably OK here since it's testing lower than that.

@clux clux added the changelog-add changelog added category for prs label Jun 7, 2022
@clux clux added this to the 0.74.0 milestone Jun 7, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2022

Codecov Report

Merging #929 (5dbc837) into master (12218ed) will increase coverage by 0.20%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #929      +/-   ##
==========================================
+ Coverage   72.24%   72.44%   +0.20%     
==========================================
  Files          64       64              
  Lines        4360     4392      +32     
==========================================
+ Hits         3150     3182      +32     
  Misses       1210     1210              
Impacted Files Coverage Δ
kube-client/src/api/mod.rs 57.14% <ø> (ø)
kube-core/src/params.rs 77.69% <100.00%> (+6.66%) ⬆️

phroggyy added 5 commits June 11, 2022 19:10
The `fieldValidation` flag should be set to `Ignore` when doing a server-side
apply for a partial object. This introduces support to configure the
fieldValidation parameter to all the available options.

Signed-off-by: Leo Sjöberg <leo.sjoberg@gmail.com>
Signed-off-by: Leo Sjöberg <leo.sjoberg@gmail.com>
Signed-off-by: Leo Sjöberg <leo.sjoberg@gmail.com>
Signed-off-by: Leo Sjöberg <leo.sjoberg@gmail.com>
Signed-off-by: Leo Sjöberg <leo.sjoberg@gmail.com>
@phroggyy phroggyy force-pushed the feat/add-field-validation-directive branch from 0748667 to ce79903 Compare June 11, 2022 17:15
Signed-off-by: Leo Sjöberg <leo.sjoberg@gmail.com>
@clux
Copy link
Member

clux commented Jun 13, 2022

This looks all good to me. Minor rustfmt thing (we are using nightly for fmt, which causes some confusion), but i'll just merge it to test if our rustfmt fixup job is working :-) (Edit: yes)

Thanks for doing this, and for addressing all the minor technical things here! Appreciate it!

@clux clux merged commit c37d07b into kube-rs:master Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-add changelog added category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants