add field selector support to K8s.Selector

[drowzy]

Took a quick stab at #40. Adds a new match_fields to K8s.Selector to hold key, values related
to the field selectors. As field selectors only work with equality-based requirements I embedded the operator in the value part of the kv-pair

Changed merge to branch depending on type of selector (:label, :field). So

iex(1)> {"component", "redis"} |> K8s.Selector.label() |> K8s.Selector.field({"metadata.namespace", "default"})
%K8s.Selector{
  match_expressions: [],
  match_fields: %{"metadata.namespace" => {"=", "default"}},
  match_labels: %{"component" => "redis"}
}

and with an operation:

iex(2)> K8s.Client.list("v1", :pods) |> K8s.Selector.label({"component", "redis"}) |> K8s.Selector.field({"metadata.namespace", "default"})
%K8s.Operation{
  api_version: "v1",
  data: nil,
  method: :get,
  name: :pods,
  path_params: [],
  query_params: [
    fieldSelector: %K8s.Selector{
      match_expressions: [],
      match_fields: %{"metadata.namespace" => {"=", "default"}},
      match_labels: %{}
    },
    labelSelector: %K8s.Selector{
      match_expressions: [],
      match_fields: %{},
      match_labels: %{"component" => "redis"}
    }
  ],
  verb: :list
}

I'm a little bit unsure what you had in mind so I thought I'll start simple. Let me know what you think!

[coryodaniel]

Hey thanks for the PR! Sorry for the late response it’s been a busy couple of months.

I think this looks good have you tested it against a cluster already?

I think removing the to_s function is going to require a version bump unless we defdelegate to label_to_s

[drowzy]

Hey thanks for the PR! Sorry for the late response it’s been a busy couple of months.

I think this looks good have you tested it against a cluster already?

Yes, a local cluster running a redis pod in default namespace:

apiVersion: v1
kind: Pod
metadata:
  name: redis
  labels:
    component: redis
spec:
  containers:
  - name: redis
    image: redis:5.0.4
    command:
      - redis-server
    env:
    - name: MASTER
      value: "true"
    ports:
    - containerPort: 6379
    resources:
      limits:
        cpu: "0.1"
  volumes:
    - name: data
      emptyDir: {}

Testing between fetching pods in default namespace and kube-system.

I think removing the to_s function is going to require a version bump unless we defdelegate to label_to_s

Ok! Should I put an @deprecated notice on to_s?

[mruoss]

I would definitely keep to_s/1 for backwards compatibility. I see two options:

• keep, delegate to labels_to_s/1 and deprecate
• keep the name to_s/1 for both and distinct the cases with the pattern matching already in place.

[drowzy]

• keep the name to_s/1 for both and distinct the cases with the pattern matching already in place.

I like this one, the behaviour for those relying on to_s/1 would be the same and support fields as well. However what would the output be in case there's both field and label selectors present in the struct? I.e:

iex(1)> {"status.phase", "Running"} |> K8s.Selector.field() |> K8s.Selector.label({"component", "redis"})
%K8s.Selector{
  match_expressions: [],
  match_fields: %{"status.phase" => {"=", "Running"}},
  match_labels: %{"component" => "redis"}
}

And how would one tell them apart? Returning a tuple would solve it but again it would require a version bump.

[mruoss]

Yeah you are right, that's not very clean either. I guess the question is: Should the same struct (%K8s.Selector{}) really be used for two so independent cases.
But I guess from where we are now, probably it's best the way you proposed with the two new functions but keep to_s/1 for bc.

[mruoss]

OR: you keep one single to_s/1 that deals with both, match_fields and match_lables and returns a keyword list that contains both. That would probably still be backwards compatible...

[mruoss]

btw: you can rebase onto the latest develop branch in order to please the dialyzer ;)

[mruoss]

OR: you keep one single to_s/1 that deals with both, match_fields and match_lables and returns a keyword list that contains both. That would probably still be backwards compatible...

@drowzy what do you think about that?

[drowzy]

OR: you keep one single to_s/1 that deals with both, match_fields and match_lables and returns a keyword list that contains both. That would probably still be backwards compatible...

Yeah, seems like a good idea. See updated PR! 😃

[mruoss]

Yes, this looks better. Might add some integrtion tests at some point...

[mruoss]

Is it okay if I take this over? I'm planning on refactoring/simplifying the selector module a bit. I'm gonna keep your commits in and build on top of them. Thanks anyway for the PR. I really want to get this feature in.

[drowzy]

Is it okay if I take this over? I'm planning on refactoring/simplifying the selector module a bit. I'm gonna keep your commits in and build on top of them. Thanks anyway for the PR. I really want to get this feature in.

It’s fine! Go ahead :)

[mruoss]

Alright, I'm done refactoring.

• removed the merge private function. Instead, I aggregate new values to passed structs. I think it makes the code much clearer.
• In operations, I keep one single selector. It is still called :labelSelector for BC but contains an aggregated selector object.
• deprecated old functions.
• added integration tests

Thanks again @drowzy for your contribution!

[drowzy]

Lgtm!