Skip to content
This repository has been archived by the owner on Sep 4, 2020. It is now read-only.

Implement the [variable(NAME)] function in YAML for ApplicationConfigurations #70

Closed
technosophos opened this issue Aug 12, 2019 · 11 comments · Fixed by #264
Closed

Implement the [variable(NAME)] function in YAML for ApplicationConfigurations #70

technosophos opened this issue Aug 12, 2019 · 11 comments · Fixed by #264
Assignees
Labels
Status: In Progress Work In Progress

Comments

@technosophos
Copy link
Contributor

For post-processing of operational configurations, there are a few places where the syntax [fromVariable(NAME)] should result in a substitution of a variable name into a value. This is described here: https://github.com/microsoft/hydra-spec/blob/master/6.operational_configuration.md#properties

@fibonacci1729 fibonacci1729 self-assigned this Aug 12, 2019
@hongchaodeng
Copy link
Member

hongchaodeng commented Aug 13, 2019

It would be super beneficial if this is an admission webhook.
I would imagine common functions like this would be grouped into a common layer that all Hydra runtimes use.

@technosophos
Copy link
Contributor Author

technosophos commented Aug 13, 2019

If it were an admission hook, then the version stored would not have the original template function anymore, right?

@fibonacci1729 fibonacci1729 added the Status: In Progress Work In Progress label Sep 14, 2019
@wonderflow wonderflow changed the title Implement the [variable(NAME)] function in YAML for OperationalConfigurations Implement the [variable(NAME)] function in YAML for ApplicationConfigurations Sep 29, 2019
@wonderflow
Copy link
Member

wonderflow commented Sep 29, 2019

Hi, @fibonacci1729 are you still working on this and going to use admission webhook? Or it should be closed by #161?

I think #178 could also be fixed by admission webhook, if you are writing this, we can use the same framework.

/cc @technosophos

@technosophos
Copy link
Contributor Author

So I think it would be fine to do this as an admission webhook for a first pass. I like that idea a lot. Is that okay with you @fibonacci1729 ? Is that something you would like to do?

@fibonacci1729
Copy link
Collaborator

fibonacci1729 commented Oct 1, 2019

My plan was to implement this in the simplest way (from file). Yesterday, I was scoping the work involved to support it as an admission hook and it seems relatively straightforward. So I'll adapt the current work to support the admission hook!

@wonderflow
@technosophos

@wonderflow
Copy link
Member

Nice!

@hongchaodeng
Copy link
Member

I'm not sure whether we need variables at all?
It seems variables is used as template rendering in AppConfig yaml. But why not make use of other tools like helm, kustomize to do that?

@technosophos
Copy link
Contributor Author

We talked about that a lot on the spec. One of the requirements we got internally was to allow internal templating without Helm. I think we got a long way with the fromParam field on Components Schematics, but ultimately this function made it into the spec: https://github.com/microsoft/hydra-spec/blob/master/6.application_configuration.md#variable

The syntax is largely derived from ARM templates, since one of the implementations was running the YAML through ARM.

We might revisit this and see if there is a way to do this in pure YAML (we looked, for example, at YAML references as one possible way).

@hongchaodeng
Copy link
Member

Hi @technosophos

Yes. This has been discussed before. And I agree that fromParam is necessary.

However, after a second thought, fromVariable is different from fromParam:

  1. fromParam is defined in Component, etc. and rendered in AppConfig. It's two phases. We need this internal rendering capability.
  2. fromVariable is defined and rendered in AppConfig. It's one phase. That's why I'm wondering if we need it?

@technosophos
Copy link
Contributor Author

Yes, you are correct. fromVariable is basically a shortcut for declaring data in one place and using it multiple places.

We probably could remove it from the spec, and then just use an external templater (helm is the obvious chose for me, but anyone could use the one they prefer).

Maybe we should move this discussion back to the spec?

/cc @suhuruli @vturecek

@hongchaodeng
Copy link
Member

Sure. That sounds great!
I will create an issue for it

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Status: In Progress Work In Progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants