core: add support for input variables

[krantzinator]

Adds support for input variables.

Docs will be in a separate PR.

Input variables are currently available within any stanzas in the waypoint.hcl; they are not available for top-level objects such as project parameter.

A couple of videos showing functionality, combined with #1658

input-vars-full-2021-06-29_15.32.04.mp4

gitops-vars-2021-06-29_11.06.34.mp4

[mitchellh]

Awesome @krantzinator! There's a ton of good stuff here.

I think my only major issue is with internal/config/variables.go. The rest of how you're parsing it, how you're using it, seems right. The style of internal/config/variables.go feels a bit weird. To define that a bit more, it feels (1) not strictly idiomatic Go (2) not Waypoint-ish Go style (3) feels like it conflates some knowledge across abstraction boundaries.

I tried to step back to think "okay, what are we doing here big picture" and it seems like what you're achieving is:

• Parse variable definitions from an HCL config (and maybe defaults)
• Load variable values from env vars
• Load variable values from files
• Load variable values from flags
• Sort by precedence

That stands out to me as each should be their own function. Right now you have CollectInputVars that seems to do both env and file, and CollectRemote which seems to assign values to definitions? (That one isn't clear to me).

Order of calling those functions doesn't seem to matter since we sort by precedence later. So I feel like internal/cli can just call the env and file load manually and separately, and the runner can call the appropriate functions as well, then sort/merge.

There's also enough here that it could make sense to put all these functions in a package like internal/config/vars. Cause I think you're probably expanding the exported surface area of the internal/config package by quite a lot and that's usually a smell to me to extract a package.

My recommendation would be to take a pass at the types/functions with this lens. If you want to hop on a Zoom happy to do that too.

[krantzinator]

Thanks @mitchellh ! I'll respond with thoughts to your overall here, and then take a look at the piece-specific suggestions.

The style of internal/config/variables.go feels a bit weird. To define that a bit more, it feels (1) not strictly idiomatic Go (2) not Waypoint-ish Go style (3) feels like it conflates some knowledge across abstraction boundaries.

I do agree with this. I am struggling to see the entire picture here, so a lot of these functions are in the vein of "make it work, make it right, make it fast" mentality; and they are all currently in the "make it work" stage with me anticipating quite a bit of refactoring as I add other pieces and come back to consolidate. "Not strictly idiomatic Go" is also definitely a thing 😄 My Ruby/JavaScript brain background comes to the forefront when I'm working out a new architecture.

Right now you have CollectInputVars that seems to do both env and file, and CollectRemote which seems to assign values to definitions? (That one isn't clear to me).

This isn't clear to me either 😅 This is still in the "Make it work" part of my brain. Definitely want to refactor this once I've got a couple more pieces ironed out.

Order of calling those functions doesn't seem to matter since we sort by precedence later. So I feel like internal/cli can just call the env and file load manually and separately, and the runner can call the appropriate functions as well, then sort/merge.

This is my plan 🙌🏻 .

There's also enough here that it could make sense to put all these functions in a package like internal/config/vars. Cause I think you're probably expanding the exported surface area of the internal/config package by quite a lot and that's usually a smell to me to extract a package.

That's how I started, and then I pulled it back into config because it felt like a too-early abstraction. I agree it feels right now to put it in its own package, or at least most of it with maybe one or two links between config and config/vars.

My recommendation would be to take a pass at the types/functions with this lens. If you want to hop on a Zoom happy to do that too.

Sure thing.

[evanphx]

Looks great! A few little odds and ends.

[mitchellh]

Great work! This is very clean and there's certainly a lot here. I didn't have any fundamentally "broken" feedback, just lots of polish points.

[mitchellh]

Just a note that this is surely a candidate for bold feature (that we don't support automating yet). So no action you have to do @krantzinator but just for us to know when we cut a release and build the changelog.

[krantzinator]

10-4

[krantzinator]

FYI: Once this is approved I'm going to rebase and pull the protobuf gen commit to the front, since I anticipate this having a hard time backporting in general, but definitely on that protobuf gen. Having that at the end to just throw away and re-gen on release/0.4.x will be much nicer.

[mitchellh]

Looking reallllllll close. Just one small comment.

[evanphx]

Looks great!

[denysvitali]

Hey! Thanks for the PR!

I was wondering if this change takes into consideration this simple use case that I'm facing, and if it's not too late it would be awesome to have it implemented before this gets merged:

Currently, the Internal Values can be used to have a better interpolation in the env block of the config stanza. Unfortunately config.internal.XYZ variables do not work outside the config stanza and are therefore a limitation as there is no proper way to use them as variables inside the waypoint.hcl file.

It would be nice to have something like your variable implementation, that can be used internally, kind of what you are doing with them having a default value, but without the possibility (or with a less obvious possibility) to change it externally.

As an example, we would like to be able to differentiate our hostnames depending on where we deploy our workload, and for that we use a jsonnet that could eventually work as follows:

env {
  MY_URL = file(jsonnetfile("${path.app}/env_urls.jsonnet", {ext_vars: {env = "${workspace.name}"}}))
}

While this could ideally work, unfortunately it requires now to specify multiple times the same file(jsonnetfile(...))) call over and over again. We would like to be able to have a way to use proper variables inside the waypoint.hcl file, so that we can simply use something like:

variables {
  env_url = jsonnetfile("${path.app}/env_urls.jsonnet", {ext_vars: {env = "${workspace.name}"}})
}
env {
  MY_URL = "https://${vars.env_url.some_key}/index.html"
  MY_SECOND_URL = "https://${vars.env_url.some_other_key}/"
}

This will definitely give us (and other people) more flexibility without being forced to use the config stanza (which only works if you are using the Waypoint Entrypoint, which in our case is not used).

---

I seem to understand that with your PR, this could be eventually be solved as follows:

variable "env_urls" {
  default = jsonnetfile("${path.app}/env_urls.jsonnet", {ext_vars: {env = "${workspace.name}"}})
  type = bool
} 

Unfortunately this will also expose the variable to the frontend and will most likely cause some problems since it's intended to be an "internal variable"

[krantzinator]

@denysvitali This sounds out of scope of this current PR. Input variables as implemented here do not support functions as values, so unfortunately the last example you give will not work.
It sounds a bit like you want support for a different type of variable system? When you mentioned:

variable implementation, that can be used internally, kind of what you are doing with them having a default value, but without the possibility (or with a less obvious possibility) to change it externally.

that makes me think the problem you are wishing to solve is different from the problem this particular feature is targeting. The entire point of input variables is to allow for values to be changed externally 🙂

[mitchellh]

@krantzinator @denysvitali This sounds more like you want local values similar to Terraform. This is something that @krantzinator and I have talked about building off of this work in the future.

Note on internal variables not being available outside of config: Evan and I talked about that annoyance for exactly the use case you ran into, but the reason we did this is because config variables are especially special in that they are "rendered" or "realized" or "loaded" (all synonymous in this case) at a different time to everything else.

[mitchellh]

I think we're ready.

[denysvitali]

@mitchellh Yes, that's exactly what I need! Thanks for the clarification!

Do you want me to open a feature request for it?

[mitchellh]

Yes please.