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

Configs per step #1499

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from
Draft

Conversation

hoegaarden
Copy link

@hoegaarden hoegaarden commented Mar 4, 2024

What this PR does / why we need it:

This allows finer grained configurations of an app's template steps from a pkgi.

  • values in a pkgi can be configured to be be attached to a specific template step in the resulting app
  • configuration of an app's template step, through annotations on the pkgi, can be configured to apply to a specific templating step

Which issue(s) this PR fixes:

Related issues / comments:

Does this PR introduce a user-facing change?

First off: the previous behavior regarding values and annotations should have kept the same, so this should be fully backwards compatible.

It introduces a couple of new and additional knobs:

  • pkgi.spec.values.templateSteps is an []int
    • it's optional and default to an empty list, which then behaves like before this change
    • pkgi.spec.values.templateSteps: [ 0 , 2 ] would attach this value secret as a value to the first and third template step
    • if a template step does not take values, or a non-existing template step is referenced, an error is produced
  • In addition to the already existing annotations1, new ones2 have been introduced, modeled after the "fetch secret annotation"3:
    • e.g. in addition to ext.packaging.carvel.dev/helm-template-name which applies to the first helm template step, ext.packaging.carvel.dev/helm-0-template-name can be used to explicitly target the first template step
    • if for a referenced template step the annotation is not relevant (a "helm annotation" for a ytt template step), the annotation is ignored, i.e. no error is produced
    • the original annotations are still used (for the first relevant templating step), again keeping the current behavior

Note: A user deploying a pkgi would still need to understand the implementation details of the pkg, i.e. which template steps it consists off, which values a certain step takes, ...


Additional Notes for your reviewer:

Review Checklist:
  • Follows the developer guidelines
  • Relevant tests are added or updated
  • Relevant docs in this repo added or updated
  • Relevant carvel.dev docs added or updated in a separate PR and there's
    a link to that PR
  • Code is at least as readable and maintainable as it was before this
    change

Additional documentation e.g., Proposal, usage docs, etc.:


Footnotes

  1. ext.packaging.carvel.dev/helm-template-name
    ext.packaging.carvel.dev/helm-template-namespace
    ext.packaging.carvel.dev/ytt-paths-from-secret-name
    ext.packaging.carvel.dev/helm-template-values-from-secret-name
    ext.packaging.carvel.dev/ytt-data-values-overlays

  2. ext.packaging.carvel.dev/helm-%d-template-name
    ext.packaging.carvel.dev/helm-%d-template-namespace
    ext.packaging.carvel.dev/ytt-%d-paths-from-secret-name
    ext.packaging.carvel.dev/helm-%d-template-values-from-secret-name
    ext.packaging.carvel.dev/ytt-%d-data-values-overlays

  3. ext.packaging.carvel.dev/fetch-%d-secret-name

Hannes Hörl added 5 commits March 15, 2024 09:25
Values used in pkgis can be configured to apply to specific template
steps of the pkg. The default behaviour stays the same, so that no
specific step is defined for data values, they apply to the first one
that takes values.

Some implementation details:

We classify the pkg's template steps, currently supported classes are
ytt, helm, cue, and "valueable" which is either of the previously
mentioned.
When it is time to apply the data values' secrets to the template steps
of the reulting app, we either find the template step that is explicitly
configured, or we find the first one that takes data values (the first
"valueable" step).

This flips the the approach around: previously we iterated through all
the pkg's template steps and applied the data values, if we have not.
Now we iterate over the the data values from the pkgi and apply them to
the appropriate template step.

If the user configured data values for invalid steps (e.g. a step that
does not exist, or a step that does not take any data values) we produce
a human readable error and therefore block the reconciliation.

Signed-off-by: Hannes Hörl <hannes.hoerl+github@snowreporter.com>
This allows to control an app's individual template steps via
annotations. It is modelled after the fetch annotations, considering the
template step's index when pulling information from the app's
annotations, e.g. `ext.packaging.carvel.dev/ytt-0-paths-from-secret-name`.

The original annotations, without the index, are still used, for the
first occurrence of the relevant template step class.

The annotations are not as "strict" as the data values from the pkgi.
E.g. when there is an annotation for a template step that either does
not exist or is not of the desired class, it does not error, the
annotation is just & silently ignored. After all, with annotations we
don't have as strict of an API.

Signed-off-by: Hannes Hörl <hannes.hoerl+github@snowreporter.com>
Signed-off-by: Hannes Hörl <hannes.hoerl+github@snowreporter.com>
Signed-off-by: Hannes Hörl <hannes.hoerl+github@snowreporter.com>
Signed-off-by: Hannes Hörl <hannes.hoerl+github@snowreporter.com>
@praveenrewar
Copy link
Member

@hoegaarden Thank you so much for working on this and apologies for not being able to look into it.
I will try to take a closer look at the PR next week, however, would you be open to creating a proposal for this for the maintainers to review. Once the proposal is approved, we can move forward with implementation. Also, feel free to join our community meetings and discuss it there.

@hoegaarden
Copy link
Author

would you be open to creating a proposal

Sure. I'll try to wip up a PR for a proposal the next couple of days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

2 participants