Extending label.Parse method to support exact match #4486

sdminonne · 2015-02-17T16:14:11Z

@bgrant0607 @smarterclayton I've a first implementation of the first step (2).
Not sure if you wish to have a look to this as a PR or you would like to
I modified labels.Parse to use an hand made recursive descendant parser. (hope it's not too much work to review it).
The previous parser was partially regexp based and it may become quickly complex to work with it.
At the same time I couldn't obtain more compact code since this "micro-language" is quite unusual:
for example it can parse something like this x in (,).

Exact match is obtained using "in".
x in (a) produces the same matching of x=a, since internal data structure is the same.
String method will produce different output:
Parse("x=a").String() -> "x=a"
Parse("x in (a)").String() -> "x in (a)".

In testing file:
Added labels.Parse in the requireMatch method to be sure (or at least for this tests) that the behavior is the same (where the requested code was not too complex).

Some modifications:
a) a (with trailing whitespace) now is matched and is valid. Skipping handling heading and trailing spaces in the exact way
b) x not in ( ) is parsed and valid as x in ( ). (insure if I'm missing something)
c) x not (,a) (with leading comma) now is parsed and valid. Again if x in (abc,) is parsable why it "x not in (,a)" should be equally parsable?
d) I'm still waiting for the @thockin and @smarterclayton about 'not in' vs "NotIn" and about capitalization (not a big deal to modify)

thockin · 2015-02-17T16:32:56Z

pkg/labels/selector.go

-//      \s+
-//
+// <exact-match-restriction> ::= ["="|"=="|"!="] VALUE_NOT_EMPTY
+// KEY is a sequence of one or more characters that does not contain: ('=', '!', '(', ')', ',')


Might be simpler to define it as we do in validation (much tighter spec)?

I tried to BNF style I found. So would you prefer a regex style based as in validation.go?

I'm fine with regex for simple token-scope docs and validation. Obviously not for the full parser :)

thockin · 2015-02-17T16:44:40Z

Needs testing somewhere for keys that fit the qualified name syntax for keys: [ DNS_SUBDOMAIN "/" ] DNS_LABEL.

Specifically, test cases like "example.com/my-key" and "my-key-with-a-long-name" and "0.1.2.domain/99" are all valid as per validation rules today.

sdminonne · 2015-02-17T17:19:50Z

@thockin I've add a very simple test here https://github.com/GoogleCloudPlatform/kubernetes/pull/4486/files#diff-b2970db202ffdb1dec2b170d88679d3bR504
I can add more.
@thockin in this PR https://github.com/GoogleCloudPlatform/kubernetes/pull/4486/files#diff-2c7bc859933c6f7822d1cb7427a14573R731 is used to validate dns-subdomain/dns-label can you confirm this is the right func?

thockin · 2015-02-17T20:53:53Z

pkg/labels/selector.go

 func validateLabelKey(k string) error {
-	if !util.IsDNSLabel(k) {
+	if !util.IsQualifiedName(k) {
 		return errors.NewFieldNotSupported("key", k)


This should be NewFieldInvalid()

sdminonne · 2015-02-18T21:50:55Z

@thockin @smarterclayton in the next hours I will submit and update to fix the less controversial issues (typo, testcases, error report function)
For the regex to validate the value I don't think is going to be an issue (except if we move to bytecode). In the meantime if you converge to something like [A-Za-z0-9_-.]* just let me know.

bgrant0607 · 2015-02-19T02:34:43Z

On not in vs. NotIn:

Our general pattern is that constants in API fields are CameCase, like NotIn (e.g., ClusterFirst), and that constants expressed as part of the URL are lowercase versions of those, with no spaces or punctuation, such as notin (e.g., replicationcontrollers). I recommend we follow that same pattern here.

WDYT, @smarterclayton ?

sdminonne · 2015-02-19T17:11:53Z

@smarterclayton @thockin @bgrant0607 PTAL
Fixed the TYPO
'in' and 'notin' now can be value of the label (modified)
Modified 'not in' to 'notin'
Added the regexp suggested by @thockin + \ char for future uses as suggested by @smarterclayton . (Addes some validation tests as well)
Hope it's better this time

bgrant0607 · 2015-02-19T17:41:49Z

pkg/labels/selector_test.go

+		{"x=a", &LabelSelector{Requirements: []Requirement{
+			getRequirement("x", In, util.NewStringSet("a"), ExactMatch, t),
+		}}, true, true},
+		{"x=a,y!=b", &LabelSelector{Requirements: []Requirement{


I would like to see a test that combines the exact-match syntax and the set-based syntax, such as:

x=a,y!=b,z in (h,i,j)

smarterclayton · 2015-02-19T18:16:08Z

This is purely a personal opinion - but I think NotIn would be ugly in general use.

Seeing "foo not in bar" or "fo in bar" seems much clearer than "foo In bar" and "foo NotIn bar". I generally prefer lower case for operator words (or caseless) before mixed case. Again, this purely an "attractiveness" argument.

On Feb 18, 2015, at 9:34 PM, Brian Grant notifications@github.com wrote:

On not in vs. NotIn:

Our general pattern is that constants in API fields are CameCase, like NotIn (e.g., ClusterFirst), and that constants expressed as part of the URL are lowercase versions of those, with no spaces or punctuation, such as notin (e.g., replicationcontrollers). I recommend we follow that same pattern here.

WDYT, @smarterclayton ?

—
Reply to this email directly or view it on GitHub.

thockin · 2015-02-19T21:17:57Z

yeah, NotIn is hurtful to me. I don't think an embedded grammar syntax
needs to follow the same rules for constants. I'd just as soon see x IN y
and x NOTIN y or something.

What about !in or !(x in y)? We have to decide if this just a small hacky
thing or an actual grammar. Do we support parens (I think no), for example.

As for keys and values: I TOTALLY agree that reusing common rules is a
good thing. I almost can't keep straight the ones we have much less new
ones.

We need to have agreement on:

label values: restrict to [A-Za-z0-9_-.]*
annotation values: no restriction

Now for keys. We could just loosen all qualified names to be
([A-Za-z0-9_-.]+/)?[A-Za-z0-9_-.]+ - this should be a strict superset of
what we allow today. We can say that convention is to use dns-compatible
domain + single token, but still allow things like FOO/B_A_R. This would
impact resource names and label keys.

Alternately we could define it as caseless - but that impacts all callers.

What think?

On Thu, Feb 19, 2015 at 10:16 AM, Clayton Coleman notifications@github.com
wrote:

This is purely a personal opinion - but I think NotIn would be ugly in
general use.

Seeing "foo not in bar" or "fo in bar" seems much clearer than "foo In
bar" and "foo NotIn bar". I generally prefer lower case for operator words
(or caseless) before mixed case. Again, this purely an "attractiveness"
argument.

On Feb 18, 2015, at 9:34 PM, Brian Grant notifications@github.com
wrote:

On not in vs. NotIn:

Our general pattern is that constants in API fields are CameCase, like
NotIn (e.g., ClusterFirst), and that constants expressed as part of the URL
are lowercase versions of those, with no spaces or punctuation, such as
notin (e.g., replicationcontrollers). I recommend we follow that same
pattern here.

WDYT, @smarterclayton ?

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub
#4486 (comment)
.

sdminonne · 2015-02-20T16:21:26Z

@bgrant0607
Added the mixed test: x=a,y!=b,z in (h,i,j).
Removed MatchType to simplify interface for NewRequirement (price is more complex String() and parseOperator).

thockin · 2015-02-20T16:56:06Z

pkg/labels/selector.go

+	switch operator {
+	case In, NotIn:
+		values, err = p.parseValues()
+	case Equal, Eequal, Nequal:


why do we support and distinguish = and == both?

thockin · 2015-02-20T16:58:39Z

I don't have any further gripes

bgrant0607 · 2015-02-20T19:02:39Z

Re. NotIn: That would just be for the API struct representation. There will be some Operator field. You're saying that the values shouldn't follow our rules about constants in the API?

thockin · 2015-02-20T19:12:34Z

Ah, I meant the stringified syntax being "NotIn" is painful. Go-level
constants is fine :)

On Fri, Feb 20, 2015 at 11:03 AM, Brian Grant notifications@github.com
wrote:

Re. NotIn: That would just be for the API struct representation. There
will be some Operator field. You're saying that the values shouldn't follow
our rules about constants in the API?

Reply to this email directly or view it on GitHub
#4486 (comment)
.

bgrant0607 · 2015-02-20T22:06:30Z

To be clear, it would be an API constant string, not just a Go type. See above comparison with ClusterFirst.

As for URL syntax, the decision is notin vs not in. The former is arguably more consistent. The latter is more readable. I initially proposed the latter, then suggested the former. I don't really have a strong opinion, I guess. In the URL, the space will need to be escaped, so not%20in vs notin.

sdminonne · 2015-02-23T14:44:42Z

@bgrant0607 modified operators to constant strings. Sorry for the delay.

sdminonne · 2015-02-23T22:03:43Z

@bgrant0607 I made a mess with my previous push, sorry hope you hadn't look at it.

bgrant0607 · 2015-02-24T00:10:23Z

@sdminonne is this ready to look at now?

sdminonne · 2015-02-24T06:05:28Z

@bgrant0607 it is. Thanks

bgrant0607 · 2015-02-25T07:07:03Z

LGTM for this round.

Please rebase so tests can run.

@thockin @smarterclayton Any more comments on this PR?

smarterclayton · 2015-02-25T07:11:33Z

pkg/labels/selector.go

+		}
+		ch = l.read()
+	}
+	if ch == 0 { // end of the string?


This is a nit (just a style thing for future pulls): go encourages using a switch whenever you have this sort of pattern - it ends up reading better. Basically go hates "else if" and wants switch. If you ever are in here again later it's a minor thing to clean up if you can.

@smarterclayton I'm aware of this "go preferred way" but I've to get rid of some old habits. I'll change it. Thanks.

smarterclayton · 2015-02-25T07:12:23Z

LGTM

sdminonne · 2015-02-25T08:43:24Z

@smarterclayton @bgrant0607 I'm going to rebase and push but all my PRs are always failing for Shippable. And I trying to get info via Details simply bring me on shippable web site without no info except the home page.

sdminonne · 2015-02-25T09:05:45Z

QED :)

bgrant0607 · 2015-02-25T16:34:17Z

Integration test failed due to a pod not scheduling before the timeout, but only for Go 1.3. Probably a flake.

Extending label.Parse method to support exact match

sdminonne mentioned this pull request Feb 17, 2015

Generalize label selectors #341

Closed

thockin reviewed Feb 17, 2015
View reviewed changes

nikhiljindal assigned thockin Feb 17, 2015

thockin reviewed Feb 17, 2015
View reviewed changes

sdminonne force-pushed the issue_341 branch from d62f4a3 to 15e70ca Compare February 19, 2015 17:05

bgrant0607 reviewed Feb 19, 2015
View reviewed changes

thockin mentioned this pull request Feb 19, 2015

Loosen label and annotation validation and related tests #4628

Closed

sdminonne force-pushed the issue_341 branch from 15e70ca to d5dba8b Compare February 20, 2015 16:16

googlebot added the cla: yes label Feb 20, 2015

thockin reviewed Feb 20, 2015
View reviewed changes

sdminonne force-pushed the issue_341 branch from d5dba8b to 3bfebd3 Compare February 23, 2015 14:43

sdminonne force-pushed the issue_341 branch from 3bfebd3 to 953b247 Compare February 23, 2015 22:02

sdminonne force-pushed the issue_341 branch from 953b247 to 87f1a89 Compare February 24, 2015 23:28

smarterclayton reviewed Feb 25, 2015
View reviewed changes

Extending label.Parse method to support exact match

0186acc

sdminonne force-pushed the issue_341 branch from 87f1a89 to 0186acc Compare February 25, 2015 08:45

bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2015

bgrant0607 added a commit that referenced this pull request Feb 25, 2015

Merge pull request #4486 from sdminonne/issue_341

e152c67

Extending label.Parse method to support exact match

bgrant0607 merged commit e152c67 into kubernetes:master Feb 25, 2015

sdminonne deleted the issue_341 branch March 26, 2015 10:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extending label.Parse method to support exact match #4486

Extending label.Parse method to support exact match #4486

sdminonne commented Feb 17, 2015

thockin Feb 17, 2015

sdminonne Feb 17, 2015

thockin Feb 17, 2015

thockin commented Feb 17, 2015

sdminonne commented Feb 17, 2015

thockin Feb 17, 2015

sdminonne commented Feb 18, 2015

bgrant0607 commented Feb 19, 2015

sdminonne commented Feb 19, 2015

bgrant0607 Feb 19, 2015

sdminonne Feb 19, 2015

smarterclayton commented Feb 19, 2015

thockin commented Feb 19, 2015

sdminonne commented Feb 20, 2015

thockin Feb 20, 2015

thockin commented Feb 20, 2015

bgrant0607 commented Feb 20, 2015

thockin commented Feb 20, 2015

bgrant0607 commented Feb 20, 2015

sdminonne commented Feb 23, 2015

sdminonne commented Feb 23, 2015

bgrant0607 commented Feb 24, 2015

sdminonne commented Feb 24, 2015

bgrant0607 commented Feb 25, 2015

smarterclayton Feb 25, 2015

sdminonne Feb 25, 2015

smarterclayton commented Feb 25, 2015

sdminonne commented Feb 25, 2015

sdminonne commented Feb 25, 2015

bgrant0607 commented Feb 25, 2015

Extending label.Parse method to support exact match #4486

Extending label.Parse method to support exact match #4486

Conversation

sdminonne commented Feb 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Feb 17, 2015

sdminonne commented Feb 17, 2015

Choose a reason for hiding this comment

sdminonne commented Feb 18, 2015

bgrant0607 commented Feb 19, 2015

sdminonne commented Feb 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Feb 19, 2015

thockin commented Feb 19, 2015

sdminonne commented Feb 20, 2015

Choose a reason for hiding this comment

thockin commented Feb 20, 2015

bgrant0607 commented Feb 20, 2015

thockin commented Feb 20, 2015

bgrant0607 commented Feb 20, 2015

sdminonne commented Feb 23, 2015

sdminonne commented Feb 23, 2015

bgrant0607 commented Feb 24, 2015

sdminonne commented Feb 24, 2015

bgrant0607 commented Feb 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Feb 25, 2015

sdminonne commented Feb 25, 2015

sdminonne commented Feb 25, 2015

bgrant0607 commented Feb 25, 2015