Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Generic wait-for-condition Condition #679
base: main
Are you sure you want to change the base?
Generic wait-for-condition Condition #679
Changes from 2 commits
cb97d2f
b296c55
b3c82cf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
for reference; in docs you can use
kube::runtime
in doc paths for consistency. i've done this everywhere in all crates to refer tokube
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.
This is a doctest, which is built before the
kube
crate is. Either way, the line is hidden from readers (by prefixing with#
).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.
I almost feel like two
&str
inputs here are better than this closure. It's a struggle to understand whycond_status_ready
needs so much wrapping from a consumer standpoint when all you are really doing here is passing in the value of an expected string.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.
That doesn't cover something like
|status| status == Some("Unknown") || status == Some("True")
. It's also worth keeping in mind thatstatus
can ultimately be any string, theTrue
/False
/Unknown
troolean is just a convention.Then again, I'm not sure those advanced use-cases are something people actually want?
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.
Maybe we should look into something like https://docs.rs/stability/0.1.0/stability/attr.unstable.html rather than prefixing fn names
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.
That kind of makes sense. I'd almost rather have this be something that the end user has to opt into specifically with a
--cfg
, rather than a feature you can inherit by accident from a dependency.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.
There is the tokio solution for this as well #508 (comment). Anyway, I've made notes about this in the big stability roadmap issue, because it's something we should solve for that.
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.
Ideally we'd get around this with a
HasConditions
trait, but that would have to be upstream in k8s-openapi to be of much helpThere 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.
I think those types of traits we'd need to define in the protobuf repo. I'll open some issues there to track.
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.
🤮
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.
hah, I thought we were going ot start with
DynamicObject
first. This is pretty gross :D