Add support for label selectors in the mock server #1158
Conversation
This supports simple label selectors (eg foo=bar). The != operator would go beyond the limits of the AttributeSet-based matching in CrudDispatcher. There was a test using labels in ConfigMapCrudTest, but it was only passing because, at the point of execution, all resources match the label selector. This change moves changes the order of assertions so that the effect of the label selector is observable. This should fix fabric8io#1125, although I haven't tested that code directly.
drigz
force-pushed
the
label-selector-support
branch
from
7933ded to
d592d2d
Compare
|
ok to test |
It was only passing before because the label selector was ignored.
| private static final String VALUE_GROUP = "(?<value>[a-zA-Z0-9-_.]+)"; | ||
| private static final Pattern LABEL_REQUIREMENT_EQUALITY = Pattern.compile(KEY_GROUP + EQUALITY_GROUP + VALUE_GROUP); | ||
|
|
||
| private HttpUrl parseUrlFromPathAndQuery(String s) { |
There was a problem hiding this comment.
Is it possible to get rid of this hard coded string?
There was a problem hiding this comment.
Apart from what @rohanKanojia suggests to have this localhost as constant I will do next things in this case. I know they might sounds weird but I have done this in several projects and works pretty well in terms of readable code.
. Create a private static final String PROTOCOL = http
. Create a private static final String HOST = localhost
. Check if s starts with / or not and in case of not, append it.
. Do HttpUrl.parse(String.format("%s://%s%s", PROTOCOL, HOST, s))
if s can contain the port as well, split the method between int port, String path it is always a better practice to not merge concepts.
| private static final String VALUE_GROUP = "(?<value>[a-zA-Z0-9-_.]+)"; | ||
| private static final Pattern LABEL_REQUIREMENT_EQUALITY = Pattern.compile(KEY_GROUP + EQUALITY_GROUP + VALUE_GROUP); | ||
|
|
||
| private HttpUrl parseUrlFromPathAndQuery(String s) { |
There was a problem hiding this comment.
Apart from what @rohanKanojia suggests to have this localhost as constant I will do next things in this case. I know they might sounds weird but I have done this in several projects and works pretty well in terms of readable code.
. Create a private static final String PROTOCOL = http
. Create a private static final String HOST = localhost
. Check if s starts with / or not and in case of not, append it.
. Do HttpUrl.parse(String.format("%s://%s%s", PROTOCOL, HOST, s))
if s can contain the port as well, split the method between int port, String path it is always a better practice to not merge concepts.
|
|
||
| AttributeSet expected = new AttributeSet(); | ||
| expected = expected.add(new Attribute("labels:name", "myname")); | ||
| Assert.assertTrue("Expected " + attributes + " to match " + expected, attributes.matches(expected)); |
There was a problem hiding this comment.
Just a comment:
The project already imports AssertJ, why use JUnit assertions which are not readable at all?
There was a problem hiding this comment.
JUnit assertions are used through the rest of the file. I'm new to the project so I couldn't say why - I just aim for consistency.
There was a problem hiding this comment.
I know I know, no worries.
As recommended by @lordofthejars. I renamed PROTOCOL to SCHEME to match the convention of HttpUrl.
* Support label selectors in mock server This supports simple label selectors (eg foo=bar). The != operator would go beyond the limits of the AttributeSet-based matching in CrudDispatcher. There was a test using labels in ConfigMapCrudTest, but it was only passing because, at the point of execution, all resources match the label selector. This change moves changes the order of assertions so that the effect of the label selector is observable. This should fix fabric8io#1125, although I haven't tested that code directly. * Fix PodCrudTest's use of label selector It was only passing before because the label selector was ignored. * Improve flexibility of URL parsing As recommended by @lordofthejars. I renamed PROTOCOL to SCHEME to match the convention of HttpUrl.
* Support label selectors in mock server This supports simple label selectors (eg foo=bar). The != operator would go beyond the limits of the AttributeSet-based matching in CrudDispatcher. There was a test using labels in ConfigMapCrudTest, but it was only passing because, at the point of execution, all resources match the label selector. This change moves changes the order of assertions so that the effect of the label selector is observable. This should fix fabric8io#1125, although I haven't tested that code directly. * Fix PodCrudTest's use of label selector It was only passing before because the label selector was ignored. * Improve flexibility of URL parsing As recommended by @lordofthejars. I renamed PROTOCOL to SCHEME to match the convention of HttpUrl.
This supports simple label selectors (eg foo=bar). The != operator would
go beyond the limits of the AttributeSet-based matching in
CrudDispatcher.
There was a test using labels in ConfigMapCrudTest, but it was only
passing because, at the point of execution, all resources match the
label selector. This change moves changes the order of assertions so
that the effect of the label selector is observable.
This should fix #1125, although I haven't tested that code directly.