bug: onMissing.action.inject confusing for multi-query

[scottd018]

All paths with multi-query should show up on action inject.

Sample Manifest (/tmp/manifest.yaml):

---
apiVersion: v1
kind: Service
metadata:
  name: postgres
  namespace: my-app
spec:
  type: ClusterIP
  ports:
    - port: 5432
  selector:
    app: postgres

Sample Instruction File (/tmp/instructions.yaml):

yamlFiles:
  - name: "apply postgres specific configuration"
    path: "/tmp/manifest.yaml"
    overlays:
      - name: "apply postgres common name and labels"
        query:
          - metadata.name
          - metadata.labels.app
        onMissing:
          action: inject
        value: potato

Output of yot -i /tmp/instructions.yaml -s:

---
apiVersion: v1
kind: Service
metadata:
  name: potato
  namespace: my-app
spec:
  type: ClusterIP
  ports:
    - port: 5432
  selector:
    app: postgres

Expected:

---
apiVersion: v1
kind: Service
metadata:
  name: potato
  namespace: my-app
  labels:
    app: potato <<<<<<<<< this should be injected
spec:
  type: ClusterIP
  ports:
    - port: 5432
  selector:
    app: postgres

[JefeDavis]

currently, inject only occurs if no results are found on the query, since metadata.name exists in your example, inject is not triggered for any of the paths, this functions as an OR statement, if any of these paths exist, don't inject. @ahuffman have any input on this one, changing this functionality gets complicated since with how we determine results right now

[scottd018]

@JefeDavis i've had time to digest this a bit. I think the confusion may be in the term query. If you are querying something, I believe that an OR statement is correct.

This may be something where you have a separate statement like:

injectPaths:
  - metadata.name
  - metadata.labels.app
  - spec.template.metadata.labels.app
...

In which case, no query is needed. You are just saying "inject it here". I think that would avoid the confusion of query with onMissing.action.inject

But it still seems a bit confusing if you are saying "query these things and if they are missing inject them" while using OR logic for the paths.

Just my random Friday thoughts on this one. :)

[JefeDavis]

@scottd018 if I remember correctly you should be able to do multiple inject paths it uses the same object as query but with tighter validation to ensure no wild cards

[ahuffman]

@JefeDavis yes, that's true. You could use a single query and many injectPaths.

I think the problem here is that the query is returning something that does in fact exist, so it does not inject it, where the intention of injectPath was to provide a means to inject something that is not there at all.

I think the easiest way to alter/override this behavior (so to reduce number of instruction operations required like in @scottd018 example) would be to add a force: option for onMissing which would need to skip any validation whether something exists or not and look like this spec-wise:

onMissing:
  action: inject
  force: true # optional field to override onMissing behavior

Let me know what you think about adding that feature @JefeDavis. It would obviously create a version update due to backward compatibility with the existing instructions spec, otherwise see below...

The alternative (no new code) solution here @scottd018 is to have a separate overlay instruction for the 1 item that currently exists in your query items. That's not nearly as convenient, but it will work.

[ahuffman]

Although on second thought, I feel that it should be handling each query in the array as a separate item, and therefore only the ones that are missing are to be injected and ones that have an existing value should be set to the desired value. So there wouldn't be a need for a force. Something ought to be awry with the logic I'd think, potentially in how the overlay instruction item is being loaded into the queue?

[scottd018]

@ahuffman I tend to agree with you. @JefeDavis and I discussed this yesterday. The problem is in the logic for the query. It's being handled how it was intended, however, I believe the intention is super confusing from a user perspective.

I think there are 1 of 2 options to solve this:

1. PREFERRED - Keep the query with multiple paths. onMissing.action.inject should inject at all paths in the query list. I believe this is the most simple/straightforward solution from a user perspective (code on the backend is a different story) and therefore would be preferred (favor user experience over backend complexity/changes)
2. Add a new statement, e.g. injectPaths or inject which would allow you to inject at a list of locations within the document. I would argue this would really get rid of the need for the onMissing.action.inject statement entirely, as injectPaths would give that desired behavior and query would give you onMissing.action.ignore behavior. The one problem I have with this is that the DSL now becomes more complicated.

[ahuffman]

onMissing.injectPath can already be a string or array, so that functionality should already exist. The problem there is that if any 1 of the queries in the array was missing it would inject at all the injectPath locations. That may or may not be a problem based on use-case.

I think the real problem is why the metadata.name is not being set with the value. That should be happening, and do think this is a valid issue.

[scottd018]

sorry. I didn't realize there was a separate injectPath under onMissing. As i understand it, that should inject at a different path. What I mean was replacing the query functionality with injectPaths for that functionality, which I do not prefer anyway. It was pure coincidence that I chose injectPaths as my example.

[ahuffman]

Essentially each item in query should be handled one at a time and be set with the desired value.

onMissing.action should only come into play if the item being queried does not exist, and the inject action should inject the value.

injectPaths is for a use case when you query something to see if it's there or not, and want to inject something elsewhere based on that result (true|false). <- that's a weird edge use-case, but it's supported.

[scottd018]

@ahuffman that is my understanding as well and I believe should be the desired outcome for optimal UX.

[JefeDavis]

@ahuffman @scottd018 just I chime in quick injectPath is also required if your query uses wildcards or filters as the inject location needs to be in dot notation to know where to actually inject