-
Notifications
You must be signed in to change notification settings - Fork 142
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
generate: fix annotations parse #296
generate: fix annotations parse #296
Conversation
pair := strings.Split(s, "=") | ||
if len(pair) != 2 { | ||
pair := strings.SplitN(s, "=", 2) | ||
if len(pair) != 2 || pair[0] == "" { |
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.
Changing from Split
→ SplitN
makes sense (allowing =
to occur in the value). But I'm not clear on this empty-string comparison here. My understanding is that the spec allows empty-string keys for annotations, and that matches your f2c88e2 commit message. But this line will lead to “incorrectly specified…” errors when the argument starts with an equals sign. The Split
→ SplitN
change may be all we need.
On 12/28/2016 04:51 AM, W. Trevor King wrote:
But I'm not clear on thMy understanding is that the spec allows empty-string keys for annotations, and that matches your f2c88e2 <f2c88e2> commit message.
Yes, spec just requests keys to be unique, `empty or not` does not matter
But, I think may be we need to change spec. Because I think empty-string keys make no sense.
But this line will lead to “incorrectly specified…” errors when the argument starts with an equals sign. The |Split| → |SplitN| change may be all we need.
If keys can start with `=`, the condition will become very complicated.
For example if users set label as `===`, we can't find out what users really want.
maybe key is `=`, value is `=`, or maybe key is empty string and value is `==`, or key is `==` and value is empty string.
So, I think we should limit one is invalid. key starts with `=` is invalid or value starts with `=` is invalid.
As env allows value starts with `=`, so I prefer to limit key can't start with `=`.
How do you think about it?
|
On Tue, Dec 27, 2016 at 05:59:15PM -0800, Ma Shimiao wrote:
But, I think may be we need to change spec. Because I think
empty-string keys make no sense.
I'm fine with that, but I think we'd want to land that in the spec
first, instead of leading with the validation tooling.
If keys can start with `=`, the condition will become very
complicated. For example if users set label as `===`, we can't find
out what users really want… As env allows value starts with `=`, so
I prefer to limit key can't start with `=`.
That limitation makes sense for environment variables, which are
canonically stored as a single name=value string [1]. Annotations, on
the other hand, are canonically stored as a JSON object [2], so keys
can unambiguously contain equals signs. I don't think it's worth the
trouble to forbid equals signs in keys in the spec. That leaves two
options for runtime-tools:
a. Don't support equals signs in keys set via ‘generate --label=…’.
This means that some keys (those containing equals signs) cannot be
set via ‘generate’. But folks who need to set those keys can
always use jq or similar.
b. Change the generate syntax to allow unambiguous equals signs in
keys. There are a number of ways you could do this (backslash
escapes, percent quoting, jq-style value setting), ….
I don't have a preference between (a) and (b).
[1]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html
[2]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc3/config.md#annotations
|
f2c88e2
to
a809a4d
Compare
I try to use ':=' as separator. I think it's specially designed, then keys/values can contain '=' or backslash escapes or some others. |
On Wed, Dec 28, 2016 at 12:39:24AM -0800, Ma Shimiao wrote:
I try to use ':=' as separator…
The point of backslash escapes (or whatever) is that they don't put
restrictions on possible key strings. For example, you could use
‘foo\=bar=baz’ to get:
"annotations": {
"foo=bar": "baz"
}
Just replacing ‘=’ with ‘:=’ still restricts the possible key strings
(how would you get ‘"foo:=bar": "baz"’?). Any syntax based on a
single literal separator is going to make portions of the spec-legal
key space unreachable. I don't think the presense of ‘=’ in
annotation keys is likely (with or without a prefixed colon), so I
don't think this is a big deal, but using ‘:=’ is not fixing the
problem.
|
a809a4d
to
fa036ec
Compare
I'm not sure how you test |
On Tue, Jan 03, 2017 at 10:03:53PM -0800, Ma Shimiao wrote:
I'm not sure how you test `foo\=bar=baz`. But I don't think it can
work, because `\=` also contains an equals sign.
Right. You'd need splitting logic like (Python):
argument = r'foo\=bar=baz'
escaped = False
for i, char in enumerate(argument):
if escaped:
escaped = False
elif char == '\\':
escaped = True
elif char == '=':
print((argument[:i], argument[i+1:]))
break
which will print:
('foo\\=bar', 'baz')
In this PR, I will just make values contain equals sign supported,
and clearly tell users keys contain equals sign are not supported.
@wking If you have any other good solutions to fix it, you can
submit a separate PR.
I'm fine with what you have in fa036ec (it's case (a) in [1]). Using
the backslash-escape approach I sketched out in Python above would
work too, and not restrict the accessible key space. But as I said in
[1], I'm fine with (a), especially since you document the restriction.
[1]: #296 (comment)
|
fa036ec
to
75be481
Compare
According to runtime-spec/#645, updated PR to add empty-string key comparison. |
With the spec change, 75be481 looks good to me. |
@@ -96,6 +96,7 @@ read the configuration from `config.json`. | |||
|
|||
**--label**=[] | |||
Add annotations to the configuration e.g. key=value. | |||
Currently, key contains equals sign is not supported. |
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.
nit: contains -> containing
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.
fixed, thanks
75be481
to
61b1b7d
Compare
It's valid for values to contain '='. Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
@mrunalp @liangchenye @hqhq PTAL |
It's valid for values to start with '=' .
Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com