[TEP-0075, 0076] Make array and object support implementable 👷‍♀️

[bobcatfish]

Update the TEPs about array and object support (TEP-0075 and TEP-0076)
to be implementable.

Also addressing feedback from @afrittoli in #479

Outstanding items:

• @afrittoli re. behavior of optional fields when combined with variable replacement ([TEP-0075] Propose object (dictionary) param and result types 🤓 #479 (comment)) - i think that attempts to do variable replacement with a value that is not provided (even if it is optional) should fail and we should lean on the ability to provide defaults (and esp TEP-0048) instead
• @afrittoli re. supporting a mode with no schema provided at all (([TEP-0075] Propose object (dictionary) param and result types 🤓 #479 (comment), [TEP-0075] Propose object (dictionary) param and result types 🤓 #479 (comment)) - can you provide an example of what that would look like? (also feels like maybe a future enhancement we could add?)
• @mogsie please raise if your concerns from [TEP-0076] Propose array results and indexing 🐠 #477 were not addressed

cc @wlynch

[jerop]

exciting! 🎉

/assign @jerop
/assign @vdemeester

[wlynch]

/assign wlynch

[afrittoli]

Update the TEPs about array and object support (TEP-0075 and TEP-0076) to be implementable.

Also addressing feedback from @afrittoli in #479

Thanks for the updates!

Outstanding items:

• @afrittoli re. behavior of optional fields when combined with variable replacement ([TEP-0075] Propose object (dictionary) param and result types 🤓 #479 (comment)) - i think that attempts to do variable replacement with a value that is not provided (even if it is optional) should fail and we should lean on the ability to provide defaults (and esp TEP-0048) instead

If we provided a syntax to check if a value is defined, it could be ok. Otherwise I don't see what purpose an optional field in an object parameter would serve, they'd be unusable since referring to an optional parameter might trigger a failure at any time and it would not be possible to build conditional logic anywhere.

To go back the my example in the previous PR:

apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: optional-fields
spec:
  params:
    - name: foo
      type: object
      properties:
        bar: {}
        needed: {}
      required: ["needed"]
  steps:
    - name: first-step
      image: alpine
      env:
      - name: FOO_BAR
        value: $(params.foo.bar)
      script: |
        #!/bin/sh
        set -ex
        set -o pipefail
        if [ -z "$FOO_BAR ]; then
          echo "foo does not have a bar, or it's empty ¯\_(ツ)_/¯"
        fi

The check above would not be possible, so the bar field would be unusable.
If we provided some safe syntax to check if a field is defined it should be implemented so that it can be used in env, since we discourage parameter substitution in scripts.

Default values do not solve this in case of objects, because defaults for objects - if I understood correctly - will only be at object level, so to make use of bar I would be forced to provide a default foo that includes both bar and needed which may not be what I want - i.e. perhaps there's not good default for needed.

• @afrittoli re. supporting a mode with no schema provided at all (([TEP-0075] Propose object (dictionary) param and result types 🤓 #479 (comment), [TEP-0075] Propose object (dictionary) param and result types 🤓 #479 (comment)) - can you provide an example of what that would look like? (also feels like maybe a future enhancement we could add?)

I'll try to produce an example, but I agree this is not blocking for this PR / initial implementation.

• @mogsie please raise if your concerns from [TEP-0076] Propose array results and indexing 🐠 #477 were not addressed

cc @wlynch

[bobcatfish]

Thanks for the example @afrittoli ! I agree that it feels strange for a field to be optional, however for any attempts to use that optional field to fail. It feels like all of the options we have for this (https://github.com/tektoncd/community/blob/main/teps/0075-object-param-and-result-types.md#required-vs-optional-keys) are not great in their own way. I don't think that list includes an option where we have "zero value" for fields that aren't specified, which I think is what you were suggesting (e.g. "" for a string field that is missing) - we should at least list that as an alternative, and maybe that's the best option but I think that pulls in TEP-0048 as well i.e. do we always do this for missing values and how do you tell the difference between "" b/c something wasn't provided and "" when explicitly provided...

Default values do not solve this in case of objects, because defaults for objects - if I understood correctly - will only be at object level

Maybe a good reason to actually support merging the default with the provided object after all?

Another idea: supporting a syntax that lets you specify a default when you use a value, e.g.:

  params:
    - name: foo
      type: object
      properties:
        bar: {}
        needed: {}
      required: ["needed"]
  steps:
    - name: first-step
      image: alpine
      env:
      - name: FOO_BAR
        value: $(params.foo.bar:"some-default")

(#240 (comment))

Anyway none of the options really seem great 😅 was discussing with @wlynch a bit today and he might have some ideas

[ywluogg]

I agree that an optional field shouldn't fail the run. I'm also voting that if the optional field is not provided, "zero" values should be inserted. I'm not sure if we have a use case for that we want to distinguish the difference between something wasn't provided and when that's explicitly provided, as optional field should be optional that this detail may not be necessary. Currently I can't think of such case in terms of optional field, but I could be missing some edge cases XD

Thanks for the example @afrittoli ! I agree that it feels strange for a field to be optional, however for any attempts to use that optional field to fail. It feels like all of the options we have for this (https://github.com/tektoncd/community/blob/main/teps/0075-object-param-and-result-types.md#required-vs-optional-keys) are not great in their own way. I don't think that list includes an option where we have "zero value" for fields that aren't specified, which I think is what you were suggesting (e.g. "" for a string field that is missing) - we should at least list that as an alternative, and maybe that's the best option but I think that pulls in TEP-0048 as well i.e. do we always do this for missing values and how do you tell the difference between "" b/c something wasn't provided and "" when explicitly provided...

Default values do not solve this in case of objects, because defaults for objects - if I understood correctly - will only be at object level

Maybe a good reason to actually support merging the default with the provided object after all?

Another idea: supporting a syntax that lets you specify a default when you use a value, e.g.:

  params:
    - name: foo
      type: object
      properties:
        bar: {}
        needed: {}
      required: ["needed"]
  steps:
    - name: first-step
      image: alpine
      env:
      - name: FOO_BAR
        value: $(params.foo.bar:"some-default")

(#240 (comment))

Anyway none of the options really seem great 😅 was discussing with @wlynch a bit today and he might have some ideas

I really think the feature to letting users specifying a default value for a non object type of field in object field access will be beneficial, and providing the defaults at variable replacement seems more intuitive, and code doesn't have to check back the schema definition of the object. We could first having support for the zero values, and plan for supporting this default value syntax.

Regarding to default values for object types, we do have the option for merging the default with the provided object, but do we really want to go down this route, since objects can be have multiple layers of nested objects? And is that a complicated feature that's useful? We also have to consider the required fields within an optional field that's a object...

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop, vdemeester, wlynch

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• teps/OWNERS [jerop,vdemeester,wlynch]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[wlynch]

Anyway none of the options really seem great 😅 was discussing with @wlynch a bit today and he might have some ideas

Summarizing from last week's API working group - it sounded like we were leaning towards making all fields required to start, and just documenting it as an unsupported field?

Another question we should figure out (though it doesn't really change the core of this proposal so isn't blocking) is whether we want to use vanilla json schema or OpenAPI spec (which is mostly similar, but diverges from the original json schema spec in a few aspects).

[ywluogg]

Realized I missed the API WG...
Thanks Billy for summarizing this up!
Does it mean we mark the optional fields as unsupported? I realized the notes are also not in detailed. I have less understandings of what we agreed on regarding to no schemas. Can you also provide more details regarding to that?

Anyway none of the options really seem great 😅 was discussing with @wlynch a bit today and he might have some ideas

Summarizing from last week's API working group - it sounded like we were leaning towards making all fields required to start, and just documenting it as an unsupported field?

Another question we should figure out (though it doesn't really change the core of this proposal so isn't blocking) is whether we want to use vanilla json schema or OpenAPI spec (which is mostly similar, but diverges from the original json schema spec in a few aspects).

[afrittoli]

/test pull-community-teps-lint

[afrittoli]

I have less understandings of what we agreed on regarding to no schemas. Can you also provide more details regarding to that?

For this specifically it was initially brought up by me - I think that's something we might consider as a follow-up feature, but I would not include it necessarily in the initial design

[afrittoli]

Anyway none of the options really seem great 😅 was discussing with @wlynch a bit today and he might have some ideas

Summarizing from last week's API working group - it sounded like we were leaning towards making all fields required to start, and just documenting it as an unsupported field?

+1

Another question we should figure out (though it doesn't really change the core of this proposal so isn't blocking) is whether we want to use vanilla json schema or OpenAPI spec (which is mostly similar, but diverges from the original json schema spec in a few aspects).

What would be the advantage of OpenAPI vs jsonschema?

[bobcatfish]

re. json schema vs OpenAPI - there's a quick blurb about this in the proposal (https://github.com/tektoncd/community/blob/main/teps/0075-object-param-and-result-types.md#why-json-schema) - my understanding is that the JSON schema features we're proposing are compatible with OpenAPI schema so we could adopt it later if there are compelling features we need

[bobcatfish]

@ywluogg :

Does it mean we mark the optional fields as unsupported?

Yes - at least initially. We can always add support for optional fields later.

I realized the notes are also not in detailed. I have less understandings of what we agreed on regarding to no schemas. Can you also provide more details regarding to that?

Yes 🙏 The most recent commit should provide more detail around the 'no schema' feature and around the optional vs required fields 080db4a

[bobcatfish]

I've update this PR with the changes we discussed in the last WG - and it looks like we've got approval from all the approvers :D

[jerop]

/kind tep

(to make sure we merge it at API WG today)

[jerop]

Approved at API WG

/lgtm