-
-
Notifications
You must be signed in to change notification settings - Fork 374
feat!: replace chrono with Jiff #1868
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
Conversation
085d4d8 to
323715a
Compare
also drops v1.30 support but we revert this in a follow-up commit before opening the PR (so let's not keep it as part of the commit message as that would be confusing). v1.30 is dropped as k8s-openapi already dropped it on `main` and we want to test against the `main` status once before reverting to the published version. Signed-off-by: ngergs <[email protected]>
k8s openapi does not support any version <1.30 so there is no need to check if we are at a version >=1.30 Signed-off-by: ngergs <[email protected]>
Signed-off-by: ngergs <[email protected]>
…enapi Signed-off-by: ngergs <[email protected]>
| #k8s_openapi::k8s_if_ge_1_30! { | ||
| let fields : Vec<#apiext::SelectableField> = #serde_json::from_str(#fields).expect("valid selectableField column json"); | ||
| } | ||
| let fields : Vec<#apiext::SelectableField> = #serde_json::from_str(#fields).expect("valid selectableField column json"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also changed this as all versions available in k8s-openapi v0.26.0 are greater or equal than v1.30 anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you may be able to shift k8s-openapi depedency to target the git ref for now (they should be releasing for a new kubernetes version soon-ish).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong thread? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regarding the k8s-openapi git dependency: do you suggest doing this just for the example or also for kube-runtime?
I had a version where it's done for both (revert/drop the last 2 commits of this PR) but that would block a kube release till k8s-openapi has a release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought was doing it for everything in the repo, given that we are unlikely to make a new kube release until the next k8s-openapi release.
...But that said, on a second thought, and given squash merging, it's perhaps better to do that in a subsequent PR. Happy to merge this withot a git pin provided the other comment is resolved!
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1868 +/- ##
=======================================
- Coverage 74.7% 74.7% -0.0%
=======================================
Files 84 84
Lines 7958 7954 -4
=======================================
- Hits 5939 5936 -3
+ Misses 2019 2018 -1
🚀 New features to boost your workflow:
|
|
Note that |
clux
left a comment
There was a problem hiding this 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. This generally looks sensible to me.
I have one comment on the new error case in log params which i am not sure i understand, and feel like we might be able to drop.
| #k8s_openapi::k8s_if_ge_1_30! { | ||
| let fields : Vec<#apiext::SelectableField> = #serde_json::from_str(#fields).expect("valid selectableField column json"); | ||
| } | ||
| let fields : Vec<#apiext::SelectableField> = #serde_json::from_str(#fields).expect("valid selectableField column json"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you may be able to shift k8s-openapi depedency to target the git ref for now (they should be releasing for a new kubernetes version soon-ish).
… our usage Also aligned all `rfc 3339` comments to call it `RFC3339` so that a string search finds them together with other mentions of this rfc. Signed-off-by: ngergs <[email protected]>
clux
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much!
Closes issue #1865. Motivation is to follow
k8s-openapiwhich did the same replacement of itsmainbranch.There are still some
chronoleftovers inkube-runtimeand the examples to make the PR compatible with the current relased version ofk8s-openapi.Reverting the last commit of this PR should remove all
chronoleftovers once a newk8s-openapiversion has been released.