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

Treat object parameters as objects in templating #3225

Merged
merged 5 commits into from
Sep 30, 2024

Conversation

lbergnehr
Copy link
Contributor

What does this change

This change makes it possible to access a parameter in templating, not only to the top-level, i.e. the parameter itself, but also -- if the parameter is of type "object" -- to access properties of the object value. For example:

parameters:
  - name: foo
    type: object
    default:
      a: 1
      b: 2

install:
  mymixin:
    some_property: ${ bundle.parameters.foo.a }

In this case, the .a was not possible before as foo was merely a JSON string at the point of template resolution.

What issue does it fix

Closes #1508 (partly?)

Notes for the reviewer

This does not solve the same issue for credentials. I'm not sure if that scenario is as common as with parameters though.

I wasn't sure how to handle sensitive along with a parameter of type object. Right now it's required for the type to be string is sensitive is to work. What would be a good way to handle it? Serialize the object to a string again and then sending it to the sensitive machinery?

Checklist

  • Did you write tests?
  • Did you write documentation?
  • Did you change porter.yaml or a storage document record? Update the corresponding schema file.
  • If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

This change makes it possible to access a parameter in templating, not
only to the top-level, i.e. the parameter itself, but also -- if the
parameter is of type "object" -- to access properties of the object
value. For example:

```yaml
parameters:
  - name: foo
    type: object
    default:
      a: 1
      b: 2

install:
  mymixin:
    some_property: ${ bundle.parameters.foo.a }
```

In this case, the `.a` was not possible before as `foo` was merely a
JSON string at the point of template resolution.

Signed-off-by: Leo Bergnéhr <[email protected]>
Copy link
Contributor

@kichristensen kichristensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lbergnehr Thank you very much for the PR. Will take a closer look.
Quickly skimming through the related issue, could you add a test that verifies this PR won't change the existing behaviour (unless such a test already exist)? See #1508 (comment)

When trating object parameters as `map`s, the interpolation of the
object itself changes to the string representation of `map` instead of
the previous JSON string representation.

In order to keep the old behaviour of interpolating the object parameter
to a JSON string, this change introduces a new type, `FormattedObject`,
which implements the [`fmt.Formatter`](https://pkg.go.dev/fmt#Formatter)
interface. The implemented `Format` function turns the object's string
representation into a JSON string, instad of the default Go
representation of a `map`.

Example:

```yaml
parameter:
  - name: foo
    type: object
    default:
      bar: 1

install:
  mymixin:
    # Interpolating a nested property of the object parameter.
    val: "${ bundle.parameter.foo.bar }" # -> `val: '1'`

    # Interpolating the object parameter directly.
    obj: "${ bundle.parameter.foo }"     # -> `obj: '{"bar":1}'`
```

A shortcoming of this implementation is that interpolating nested
properties which also are `map` types will result in the interpolated
value being the string representation of `map`.

Signed-off-by: Leo Bergnéhr <[email protected]>
@lbergnehr lbergnehr force-pushed the interpolate-objects-in-templates branch from 78fa7f1 to 0511ee1 Compare September 25, 2024 09:55
@lbergnehr
Copy link
Contributor Author

@lbergnehr Thank you very much for the PR. Will take a closer look. Quickly skimming through the related issue, could you add a test that verifies this PR won't change the existing behaviour (unless such a test already exist)? See #1508 (comment)

Thanks @kichristensen! I added the test and also had to change the implementation to make it pass (great test! :)). There's a caveat in that nested properties will not be interpolated as JSON, but the actual parameter object will (see commit message for more info).

(FYI: the force push was only to add my signature.)

@kichristensen
Copy link
Contributor

Thanks for the update @lbergnehr. I will double-check that the behaviour is the same as in the current version, mean while could you take a look at the failing "Vet and Lint"?

Fixes lint error `SA1006`.

Signed-off-by: Leo Bergnéhr <[email protected]>
@lbergnehr lbergnehr force-pushed the interpolate-objects-in-templates branch from 7eeb595 to 3cb1f10 Compare September 30, 2024 06:49
@lbergnehr
Copy link
Contributor Author

Thanks for the update @lbergnehr. I will double-check that the behaviour is the same as in the current version, mean while could you take a look at the failing "Vet and Lint"?

Thanks for having a look @kichristensen! I think I fixed the linting error (although I couldn't run mage lint locally to verify).

This adds support for tracking object parameters as sensitive. However,
if interpolating sub-properties of an object in templating, those will
not get tracked as sensitive.

Signed-off-by: Leo Bergnéhr <[email protected]>
@lbergnehr lbergnehr force-pushed the interpolate-objects-in-templates branch from cd0d0e6 to 7630bc3 Compare September 30, 2024 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Direct way of consuming parameters/credentials as JSON objects?
3 participants