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

Update json-patch to 2.0.0 #1507

Merged
merged 1 commit into from
Jun 6, 2024
Merged

Update json-patch to 2.0.0 #1507

merged 1 commit into from
Jun 6, 2024

Conversation

bobsongplus
Copy link
Contributor

Motivation

Fix: #1503

Solution

Copy link

codecov bot commented May 31, 2024

Codecov Report

Attention: Patch coverage is 0% with 13 lines in your changes missing coverage. Please review.

Project coverage is 75.0%. Comparing base (6ce3978) to head (78a4cd6).
Report is 1 commits behind head on main.

Current head 78a4cd6 differs from pull request most recent head e20f0a5

Please upload reports for the commit e20f0a5 to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1507     +/-   ##
=======================================
- Coverage   75.1%   75.0%   -0.0%     
=======================================
  Files         78      78             
  Lines       6864    6871      +7     
=======================================
  Hits        5150    5150             
- Misses      1714    1721      +7     
Files Coverage Δ
kube-core/src/admission.rs 61.2% <ø> (ø)
kube-runtime/src/finalizer.rs 0.0% <0.0%> (ø)

... and 1 file with indirect coverage changes

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. Generally looks sensible 👍

Left a few comments. Need to be a bit careful here because it is kind of manually tested.
There is an example secret_syncer that can test this (along with its configmap that can be applied/deleted while it is running), are you able to run it to see if it still works?

kube-runtime/src/finalizer.rs Outdated Show resolved Hide resolved
kube-runtime/src/finalizer.rs Outdated Show resolved Hide resolved
kube-runtime/src/finalizer.rs Outdated Show resolved Hide resolved
@bobsongplus
Copy link
Contributor Author

Thanks a lot for this. Generally looks sensible 👍

Left a few comments. Need to be a bit careful here because it is kind of manually tested. There is an example secret_syncer that can test this (along with its configmap that can be applied/deleted while it is running), are you able to run it to see if it still works?

Thanks for taking the time to review so quickly. I will test it to see if it works or not.

@clux clux added dependencies upgrades to dependencies changelog-change changelog change category for prs labels Jun 2, 2024
@bobsongplus bobsongplus changed the title Update json-patch to 2.0.0 WIP: Update json-patch to 2.0.0 Jun 3, 2024
@bobsongplus bobsongplus changed the title WIP: Update json-patch to 2.0.0 Update json-patch to 2.0.0 Jun 3, 2024
@bobsongplus bobsongplus changed the title Update json-patch to 2.0.0 WIP: Update json-patch to 2.0.0 Jun 3, 2024
@bobsongplus
Copy link
Contributor Author

@clux thanks for taking some time to review again. All comments are fixed.

there are some test errors needed to be fixed .
/hold

@bobsongplus bobsongplus changed the title WIP: Update json-patch to 2.0.0 Update json-patch to 2.0.0 Jun 3, 2024
@bobsongplus
Copy link
Contributor Author

@clux thanks for taking some time to review again. All comments are fixed.

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 again. Looks more sensible. Just 2 small nits here for dependency optimisation and doc cleanup.

kube-core/Cargo.toml Outdated Show resolved Hide resolved
kube-core/src/admission.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.

Perfect thanks a lot!

@clux
Copy link
Member

clux commented Jun 5, 2024

(i see rustfmt is technically red, if you have a minute to re-run it, that'd be great, otherwise i'll merge it tomorrow and have ci autocorrect it)

@bobsongplus
Copy link
Contributor Author

(i see rustfmt is technically red, if you have a minute to re-run it, that'd be great, otherwise i'll merge it tomorrow and have ci autocorrect it)

Yeah, sorry for that I forgot to execute just fmt before committing. the formatted code committed.

@clux clux merged commit 5aa8f83 into kube-rs:main Jun 6, 2024
16 checks passed
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 dependencies upgrades to dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update json-patch to 2.0.0
2 participants