Skip to content

Entry API - #811

Merged
nightkr merged 12 commits into
kube-rs:masterfrom
nightkr:feature/entry
Feb 14, 2022
Merged

nightkr merged 12 commits into
kube-rs:masterfrom
nightkr:feature/entry

Conversation

@nightkr

@nightkr nightkr commented Feb 7, 2022 •

Copy link
Copy Markdown
Member

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>
Comment thread kube-client/src/api/entry.rs Outdated
Comment thread kube-client/src/api/entry.rs Outdated
@clux clux added the changelog-add changelog added category for prs label Feb 8, 2022

@clux clux left a comment

Copy link
Copy Markdown
Member

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

Comment thread kube-client/src/api/entry.rs Outdated
Comment thread kube-client/src/api/entry.rs Outdated
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

codecov-commenter commented Feb 11, 2022 •

Copy link
Copy Markdown

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
Comment thread kube-client/src/api/entry.rs
Comment thread kube-client/src/api/entry.rs Outdated
Comment thread kube-client/src/api/entry.rs
Comment thread kube-client/src/api/entry.rs Outdated

@clux clux left a comment •

Copy link
Copy Markdown
Member

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

@clux clux left a comment

Copy link
Copy Markdown
Member

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

Comment thread kube-client/src/api/entry.rs Outdated
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
Comment thread kube-client/src/api/entry.rs
Comment thread kube-client/src/api/entry.rs
@nightkr
nightkr requested a review from clux February 14, 2022 13:13

@clux clux left a comment

Copy link
Copy Markdown
Member

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