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

Entry API #811

Merged
merged 12 commits into from
Feb 14, 2022
Merged

Entry API #811

merged 12 commits into from
Feb 14, 2022

Conversation

nightkr
Copy link
Member

@nightkr nightkr commented Feb 7, 2022

Motivation

Fixes #810

Solution

Introduces a new method Api::entry, analogous to std's HashMap::entry, allowing for a (hopefully) ergonomic API for get-and-create and get-and-modify use cases.

The API should be similar to HashMap::entry, but adds an explicit Entry::sync() method for writing back any changes to K8s, allowing the user to control how and when batching happens.

@nightkr nightkr requested a review from clux February 7, 2022 11:05
@nightkr nightkr marked this pull request as draft February 7, 2022 11:05
Fixes kube-rs#810

Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
kube-client/src/api/entry.rs Outdated Show resolved Hide resolved
kube-client/src/api/entry.rs Outdated Show resolved Hide resolved
@clux clux added the changelog-add changelog added category for prs label Feb 8, 2022
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.

very clean in general, left one minor nit, but also a more future oriented problem with using replace

kube-client/src/api/entry.rs Outdated Show resolved Hide resolved
kube-client/src/api/entry.rs Outdated Show resolved Hide resolved
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
@codecov-commenter
Copy link

codecov-commenter commented Feb 11, 2022

Codecov Report

Merging #811 (3c20ff5) into master (cbf5f91) will decrease coverage by 0.11%.
The diff coverage is 72.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #811      +/-   ##
==========================================
- Coverage   70.08%   69.96%   -0.12%     
==========================================
  Files          58       59       +1     
  Lines        3984     4145     +161     
==========================================
+ Hits         2792     2900     +108     
- Misses       1192     1245      +53     
Impacted Files Coverage Δ
kube-client/src/api/mod.rs 57.14% <42.10%> (-29.53%) ⬇️
kube-client/src/api/entry.rs 75.79% <75.79%> (ø)
kube-client/src/config/mod.rs 50.00% <0.00%> (-7.15%) ⬇️
kube-client/src/client/mod.rs 70.25% <0.00%> (ø)
kube-client/src/api/core_methods.rs 86.66% <0.00%> (+2.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbf5f91...3c20ff5. Read the comment docs.

@nightkr nightkr requested a review from clux February 11, 2022 12:20
@nightkr nightkr marked this pull request as ready for review February 11, 2022 12:21
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
@nightkr nightkr self-assigned this Feb 11, 2022
@nightkr nightkr added the api Api abstraction related label Feb 11, 2022
kube-client/src/api/entry.rs Outdated Show resolved Hide resolved
kube-client/src/api/entry.rs Outdated Show resolved Hide resolved
kube-client/src/api/entry.rs Show resolved Hide resolved
kube-client/src/api/entry.rs Outdated Show resolved Hide resolved
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.

This is looking very professional, had a good go over now and could only find a few nits on documentation. The integration tests are very good (i see the coverage has bounced back from the earlier pr state 👍), and presumably these are pretty fast since it's just configmap mutations.

some unknowns on the potential error cases where people manually set metadata.{name,namespace,generate_name} but there's already a thread for it

@clux clux added this to the 0.69.0 milestone Feb 14, 2022
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
clux
clux previously approved these changes Feb 14, 2022
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.

had a quick look at the generated docs, looks great!

@clux clux dismissed their stale review February 14, 2022 12:07

ah, actually, we still have some error cases to decide on

kube-client/src/api/entry.rs Outdated Show resolved Hide resolved
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
kube-client/src/api/entry.rs Show resolved Hide resolved
kube-client/src/api/entry.rs Show resolved Hide resolved
@nightkr nightkr requested a review from clux February 14, 2022 13:13
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.

looks very clean now, no other comments on this.
merge away at your leisure, and please open up any follow-up enhancement issue ideas (like the filthyref idea)

@nightkr nightkr merged commit c6918da into kube-rs:master Feb 14, 2022
@nightkr nightkr deleted the feature/entry branch February 14, 2022 13:22
@nightkr nightkr restored the feature/entry branch February 14, 2022 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Api abstraction related changelog-add changelog added category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Entry API
3 participants