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

Deprecate ResourceExt::name in favour of safe name_* alternatives #945

Merged
merged 8 commits into from
Jul 3, 2022

Conversation

clux
Copy link
Member

@clux clux commented Jul 1, 2022

Due to the general awkwardness of having a potentially panicking ::name() method front and center even though it is generally safe to use. See #634 for initial discussion, and #942 for the most recent confusion.

This adds:

  • ResourceExt::name_any providing a short & safe drop-in replacement for name() (can return empty string) for loggers
  • ResourceExt::name_unchecked where we explicitly want a crash on missing name (like internally in runtime::reflector)

Now the relevant docs from ResourceExt looks like:

Screenshot from 2022-07-02 15-34-41

Then ran a lot of fastmod to replace in the codebase. Have set the removal of ::name to the deprecation length we used for runtime's try_flattens; removing on the 4th minor.

Plan

Long term we can remove .name() and maybe reintroduce it later as an option shorthand for .meta().name instead.

edited out comments

It is also possible we can make ::name a non-panicing variant:

self.name.unwrap_or_else(||self.generateName).unwrap_or("<unknown>")

Not sure if this is wise. But it would be nice to have something short for all the standard log gunk where this stuff is inevitably used most of the time.

EDIT: this is added under ::name_any

EDIT: removedResourceExt::name_or_generatename.

Also add name_or_generatename returning an optional.

Long term want to remove name and maybe reintroduce it as an optional
shorthand instead.

Signed-off-by: clux <sszynrae@gmail.com>
@clux clux linked an issue Jul 1, 2022 that may be closed by this pull request
@clux clux added the changelog-change changelog change category for prs label Jul 1, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2022

Codecov Report

Merging #945 (adf2875) into master (9f1df5e) will decrease coverage by 0.03%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #945      +/-   ##
==========================================
- Coverage   72.47%   72.43%   -0.04%     
==========================================
  Files          64       64              
  Lines        4399     4412      +13     
==========================================
+ Hits         3188     3196       +8     
- Misses       1211     1216       +5     
Impacted Files Coverage Δ
kube-core/src/admission.rs 61.11% <ø> (ø)
kube-core/src/resource.rs 46.51% <33.33%> (-0.99%) ⬇️
kube-core/src/params.rs 77.69% <100.00%> (ø)
kube-runtime/src/reflector/object_ref.rs 67.07% <100.00%> (ø)
kube-runtime/src/wait.rs 69.81% <0.00%> (-1.89%) ⬇️
kube-client/src/client/mod.rs 68.55% <0.00%> (+0.60%) ⬆️
kube-client/src/client/upgrade.rs 81.48% <0.00%> (+3.22%) ⬆️

Signed-off-by: clux <sszynrae@gmail.com>
@clux clux marked this pull request as ready for review July 1, 2022 08:58
clux added 2 commits July 1, 2022 13:50
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@clux clux added this to the 0.74.0 milestone Jul 1, 2022
@clux clux changed the title Deprecate ResourceExt::name in favour of name_unchecked Deprecate ResourceExt::name in favour of safe name_* alternatives Jul 1, 2022
@nightkr
Copy link
Member

nightkr commented Jul 1, 2022

Hm, not sure I see a lot of value in exposing name_or_generatename separately from name_any. It might also be worth adding a tag to name_any for the fallback cases, so we get something like my-deployment-abcde-abcde (name is present), my-deployment-abcde-(id) (fallback to generate_name), or (unnamed) (neither name nor generate_name are present).

@clux
Copy link
Member Author

clux commented Jul 1, 2022

Well, that's also one of the reasons i exposed name_or_generatename so that users can do:

obj.name_or_generatename().unwrap_or_else(||"(unnamed)") 

or whatever constant they want. i think the majority use case for the _any case is going to be quick logging rather than precise defaults, so am not super keen on making the weird use case a mandatory thing to think about. empty string returned on both missing is kind of ok when we are being imprecise - as implied by the _any suffix.

@clux
Copy link
Member Author

clux commented Jul 1, 2022

maybe a ::name_or("(unnamed)") would be sensible rather than name_or_generatename?

@nightkr
Copy link
Member

nightkr commented Jul 1, 2022

Yeah, I agree that it's kind of a weird special case. I just don't see what other use case there is where you don't care at all about the distinction between name and genname.

clux added 2 commits July 2, 2022 14:09
probably not the greatest use case for it

Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@clux
Copy link
Member Author

clux commented Jul 2, 2022

Yeah, that's a good point. I have removed name_or_generatename. If people don't care about whether they get name or generateName, they probably can just use name_any.

kube-core/src/resource.rs Outdated Show resolved Hide resolved
examples/dynamic_pod.rs Show resolved Hide resolved
left the eq/partialeq ones, those felt a bit more unclear if they were
good.

Signed-off-by: clux <sszynrae@gmail.com>
@nightkr
Copy link
Member

nightkr commented Jul 3, 2022

Both the warned-about types should be safe to derive Eq for as far as I can tell.

Signed-off-by: clux <sszynrae@gmail.com>
@clux clux merged commit 55bea0b into master Jul 3, 2022
@clux clux deleted the name_unchecked branch July 3, 2022 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-change changelog change category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admission Controller example panics on objects with no name
3 participants