rust/treefile: support templating of treefile variables

[dustymabe]

This will allow us to do something like:

variables:
  id: "fedora"
  releasever: "42"
  osversion: "${id}-${releasever}"

Which can definitely be useful in certain situations.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[dustymabe]

opened as draft to gather feedback on the implementation. still learning rust and not confident in everything I'm doing.

[jlebon]

Ask seems reasonable, though how about we structure it differently so it's instead keyed into the include mechanism, where vars in included manifests could be templated using vars defined in higher level manifests. So e.g. in your example, osversion would be in a lower shared manifest.

That naturally also allows "higher-level templating" where templated vars could themselves use templated vars, which would be pretty hard to do non-hackily I think using the current approach.

[dustymabe]

Ask seems reasonable, though how about we structure it differently so it's instead keyed into the include mechanism, where vars in included manifests could be templated using vars defined in higher level manifests. So e.g. in your example, osversion would be in a lower shared manifest.

That seems reasonable but I kind of think it's an arbitrary limitation. For example, where I wanted to use this is shown in coreos/fedora-coreos-config@54d81e6

variables:
  id: "fedora"
  osversion: "${id}-${releasever}"

in which case one of the vars would have been specified in a higher level manifest, but the other one was just defined.

That naturally also allows "higher-level templating" where templated vars could themselves use templated vars, which would be pretty hard to do non-hackily I think using the current approach.

ehh. I did think about the "templated vars could themselves use templated vars" use case but decided to cut myself off yesterday before I spent too much time on it. The idea I had there was some form of calling process_base_vars() recursively until either:

1. all vars are compliant (with no templating tokens in them any longer) -> SUCCESS
2. No vars were successfully templated in this invocation of the function -> FAILURE

In fact recursion in mind is part of the reason I convert self.base.variables into a HashMap<String, String> before calling process_base_vars() and also why I pass in substvars to be modified by the function rather than using a returned value. It was all because I was thinking originally i was going to call it recursively.

Should I pursue that?

[jlebon]

That seems reasonable but I kind of think it's an arbitrary limitation. For example, where I wanted to use this is shown in coreos/fedora-coreos-config@54d81e6

variables:
  id: "fedora"
  osversion: "${id}-${releasever}"

in which case one of the vars would have been specified in a higher level manifest, but the other one was just defined.

Though does that limit you in any way? Especially if you're dealing with conditionals, it's exactly in lower-level shared manifests that you would use them.

ehh. I did think about the "templated vars could themselves use templated vars" use case but decided to cut myself off yesterday before I spent too much time on it. The idea I had there was some form of calling process_base_vars() recursively until either:
1. all vars are compliant (with no templating tokens in them any longer) -> SUCCESS
2. No vars were successfully templated in this invocation of the function -> FAILURE

I thought about exactly this and didn't like how that would work, hence the "non-hackily" comment. :)

Not strongly opposed to be clear, but also it's not clear to me we actually need that logic anyway vs making it include-based.

But actually... I think doing it include-based is probably the only sane choice because of how it plays into conditional includes. Those are evaluated at parsing time using the vars found so far, and so you wouldn't be able to use a templated var in your conditional include conditions if they're only processed at the end.

[cgwalters]

No opposition to this in principle however I am medium-strongly of the opinion that in the --source-root case we should automatically scrape in the os-release values into treefile variables.

And as we go towards container-native builds I think --source-root becomes the default. It just doesn't make sense for each treefile user to define the os variables on their own right?

We could namespace this like ${osrelease.id}-${osrelease.version_id} to avoid clashing with anything already defined.

And we should definitely streamline a conditional (like what exists in fedora rpm macros) to do the common thing like if rhel && rhel > 10 etc.

[jlebon]

No opposition to this in principle however I am medium-strongly of the opinion that in the --source-root case we should automatically scrape in the os-release values into treefile variables.

Yup, we already do this in the node image: https://github.com/openshift/os/blob/4f1036bd11949b5901f523029abf899fd3b5673d/build-node-image.sh#L38

And it would make sense to do this in FCOS too.

[cgwalters]

The draft PR is totally fine but it'd be helpful to have unit tests just to help understand what this looks like.

[dustymabe]

I probably won't be able to get back to this until mid June since I've got some travel coming up and some prep to do for that.