Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow access to contained document for conditional validations #733

Closed
4 tasks done
Tracked by #561
pivotaljohn opened this issue Sep 3, 2022 · 0 comments · Fixed by #741
Closed
4 tasks done
Tracked by #561

Allow access to contained document for conditional validations #733

pivotaljohn opened this issue Sep 3, 2022 · 0 comments · Fixed by #741
Assignees

Comments

@pivotaljohn
Copy link
Contributor

pivotaljohn commented Sep 3, 2022

As a Configuration Author,
I want the option to reference the final Data Values within my logic for determining if the validation should be run
So that I can succinctly express intra-document dependencies with minimal cognitive load for me, those who read my schema, and those who maintain them.

For example (simplified from original):

#@data/values-schema
---
credential:
  useDefaultSecret: true
  #@schema/nullable
  #@schema/validation not_null=True, when=lambda parent: parent["useDefaultSecret"]
  secretContents:
    cloud: ""
backupStorageLocation:
  spec:
    #@schema/nullable
    #@schema/validation not_null=True, when=lambda doc: not doc["credential"]["useDefaultSecret"]
    existingSecret: ""

Here:

  • credential.secretContents is null, by default, but must be specified if credential["useDefaultSecret"] is true
  • backupStorageLocation.spec.existingSecret, likewise is null by default; but must be specified if credential["useDefaultSecret"] is false.
  • if there is also a when= condition on a validation, both when= and when_doc= must execute successfully and produce a True result; if either is False, do short-circuit running any validation rules.

Suggested tasks:

  • implement the when=
  • document this keyword in the @schema/validation reference
  • document the use of this keyword in the "Writing Validations" guide
    • "Conditional Validations" — mention use/trade-offs
    • "Mutually Exclusive Sections" — as an alternative to one_not_null= for when removing the discriminator is a breaking change.
  • update the ytt Validations Proposal to include this keyword and rationale

Context

During usability testing (#707), vetting the feature set included in this MVP was determined to be infeasibly difficult to port existing validation code to the ytt Data Values Validation mechanism.

In Tanzu Community Edition packages, alone, multiple validation rules are based on values in semantically different branches of the data values schema:

All of these cases could be partially address by hoisting the validation rules to the lowest common ancestor node in the YAML document. However, in practice this results in:

  • the logic for validation a Data Value is hoisted significantly some distance from that Data Value;
  • there is only one option for expressing conditional logic within the annotation (i.e. via the when= keyword argument) and so for these cases, multiple rules would need to include situation-specific short-circuiting in order to meet feature parity of what's currently present;
  • requiring the Author to write custom rules (instead of being able to rely on the "named" rules), pushing this kind of configuration towards being more complex.

Illustrating with the given example, above; with the current feature set, this is how that logic would be expressed:

#@data/values-schema
#@schema/validation ("backupStorageLocation.spec.existingSecret is specified", lambda value: value["backupStorageLocation"]["spec"]["existingSecret"] != None), when=lambda value: not value["credential"]["useDefaultSecret"]
---
#@schema/validation ("secretContents is specified", lambda value: value["secretContents"] != None), when=lambda value: value["useDefaultSecret"]
credential:
  useDefaultSecret: true
  #@schema/nullable
  secretContents:
    cloud: ""
backupStorageLocation:
  spec:
    #@schema/nullable
    existingSecret: ""

... all of which represents interface/usability complexity hoisted on the Author.

However, by providing access to the contained document, Authors are able to:

  • express these conditions within a validation directly on the Data Value being verified;
  • apply generalized rules without modification or adaptation on said Data Values.

... all of which amounts to a much more maintainable schema.

As a trade-off, Authors will now have the ability to hard-code locations of values within the Data Values document; when the document is reorganized, all of those references must be updated as well.
As illustrated above, this explicit dependency will exist regardless of whether the reference occurs at the root of the document or within a sub-node.

@pivotaljohn pivotaljohn mentioned this issue Sep 3, 2022
19 tasks
@carvel-bot carvel-bot added this to Carvel Sep 3, 2022
@carvel-bot carvel-bot moved this to To Triage in Carvel Sep 3, 2022
@pivotaljohn pivotaljohn moved this from To Triage to Prioritized Backlog in Carvel Sep 3, 2022
@pivotaljohn pivotaljohn moved this from Prioritized Backlog to In Progress in Carvel Sep 8, 2022
@pivotaljohn pivotaljohn self-assigned this Sep 8, 2022
Repository owner moved this from In Progress to Closed in Carvel Sep 13, 2022
@pivotaljohn pivotaljohn reopened this Sep 13, 2022
Repository owner moved this from Closed to To Triage in Carvel Sep 13, 2022
@pivotaljohn pivotaljohn moved this from To Triage to In Progress in Carvel Sep 13, 2022
@github-actions github-actions bot added the carvel triage This issue has not yet been triaged for relevance label Sep 13, 2022
@pivotaljohn pivotaljohn added in progress Work has begun by a community member or a maintainer; this issue may be included in a future release and removed carvel triage This issue has not yet been triaged for relevance labels Sep 13, 2022
@pivotaljohn pivotaljohn moved this from In Progress to Done in Carvel Sep 14, 2022
Repository owner moved this from Done to Closed in Carvel Sep 14, 2022
@vmunishwar vmunishwar added cla-not-required and removed in progress Work has begun by a community member or a maintainer; this issue may be included in a future release labels May 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants