Create mountpoint.Args for parsing and accessing Mountpoint args
#349
Conversation
Signed-off-by: Burak Varlı <[email protected]>
Signed-off-by: Burak Varlı <[email protected]>
Signed-off-by: Burak Varlı <[email protected]>
Signed-off-by: Burak Varlı <[email protected]>
unexge
force-pushed
the
add-mountpoint-args-struct
branch
from
c5af302 to
90245ac
Compare
| } | ||
| // add the hard coded S3 CSI driver user agent string | ||
| return append(options, userAgentPrefix+"="+userAgent) | ||
| func addUserAgentToArguments(args mountpoint.Args, userAgent string) mountpoint.Args { |
There was a problem hiding this comment.
We can remove this function now
There was a problem hiding this comment.
Addressed with e35a629
pkg/mountpoint/args.go
Outdated
| // Ex: --key=value, key=value | ||
| key, value = parts[0], parts[1] | ||
| } else { | ||
| // Ex: --key value, key value |
There was a problem hiding this comment.
This section doesn't make sense to me what it's meant to catch. Don't we only support one key/value pair per argument?
There was a problem hiding this comment.
Yes we only support one argument per line, the comments lists some example usages which would fall into the associated blocks. Here, this block is if the user provide arguments like: --key value or key value (without -- prefix) without using = between key and value.
There was a problem hiding this comment.
Ah, could you make the comments '--key value, or key value'? Or some other way to make it obvious the comma isn't part of the example
There was a problem hiding this comment.
Addressed with 21a0fe0
| } | ||
|
|
||
| // SortedList returns ordered list of normalized arguments. | ||
| func (a *Args) SortedList() []string { |
There was a problem hiding this comment.
Nit: Why do we need this? If it's just for equality, couldn't we return a set? (Presumably this isn't called enough to care about time complexity though)
There was a problem hiding this comment.
It's used while passing these arguments to Mountpoint via systemd or via exec.Command and also in tests as you mentioned
| // find tries to find given key from [Args], and returns whole entry, and whether the key was found. | ||
| func (a *Args) find(key ArgKey) (Arg, bool) { | ||
| key = normalizeKey(key) | ||
| for _, arg := range a.args.UnsortedList() { |
There was a problem hiding this comment.
Can you remind me again why we can't use a hashmap here and avoid doing this iteration?
There was a problem hiding this comment.
It's to support multi arguments with the same key. I think iteration wouldn't matter here much as we'd be dealing with a dozen arguments at max, but also not sure if we have a multi argument use-case with Mountpoint currently. It was just to create an ideal argument parser, might not be needed with Mountpoint args.
There was a problem hiding this comment.
If this is for multi-arguments, then this find will only find the first occurrence, which doesn't feel right. I'm happy to approve though
There was a problem hiding this comment.
Yeah that's correct, I renamed Insert method to Set which should make it clear for now I think. If we need to support multi arguments we'd also add Insert and revisit the semantics I guess.
| name: "with multiple spaces", | ||
| input: []string{ | ||
| "--allow-other", | ||
| "--uid 1000", |
There was a problem hiding this comment.
Should probably test spaces after as well
There was a problem hiding this comment.
Addressed with 41c1418
Signed-off-by: Burak Varlı <[email protected]>
|
Also added a new commit, Use mountpoint.Args in CredentialProvider, the whole |
Signed-off-by: Burak Varlı <[email protected]>
| // find tries to find given key from [Args], and returns whole entry, and whether the key was found. | ||
| func (a *Args) find(key ArgKey) (Arg, bool) { | ||
| key = normalizeKey(key) | ||
| for _, arg := range a.args.UnsortedList() { |
There was a problem hiding this comment.
If this is for multi-arguments, then this find will only find the first occurrence, which doesn't feel right. I'm happy to approve though
Previously, accessing and manipulating of Mountpoint arguments was spread over to the codebase. This new mountpoint package contains all the logic related to Mountpoint arguments.
Splitted out of #328.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.