-
Notifications
You must be signed in to change notification settings - Fork 2k
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
dynamic host volumes: test client RPC and plugins #24535
Conversation
5d87cf0
to
6006775
Compare
6006775
to
0907adc
Compare
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.
LGTM!
@@ -167,7 +171,7 @@ func (hv *HostVolume) Validate() error { | |||
} | |||
} | |||
|
|||
return mErr.ErrorOrNil() | |||
return helper.FlattenMultierror(mErr.ErrorOrNil()) |
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.
Nice QoL catch.
|
||
plug := &HostVolumePluginMkdir{ | ||
ID: "test-mkdir-plugin", | ||
TargetPath: tmp, |
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 makes me realize that calling this field "target path" is maybe a little misleading, as it's the parent of the eventual target path.
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.
yeah, aside from "target", even "path" is ambiguous: could signify either a file or a dir. I'll be in this vicinity again with fingerprinting, so I'll keep this in mind.
adding tests for #24497
also happened to notice we weren't validating that volume ID was uuid-shaped, so add that too.
main issue: #24479