Common packer vars and var validation

[codenrhoden]

This PR has 2 commits.

The first PR fixes an issue seen where the Azure builder is missing the http_proxy variable. This was in the same vein as #335, and I'm still not quite sure why this happened for some users (@dvonthenen) and not others. The previously observed behavior had always been that if you used {{user `some_var`}} and that var hadn't been defined, it ended up as an empty string. The errors that @dvonthenen say indicated otherwise. maybe it was a Packer version or something... Anyways, it has long bothered me that several of these variables were defined in multiple places, so I moved what could be moved into a common location. That's what this commit does. See commit message for a description of what couldn't be moved.

The 2nd commit gives us a way to validate the config using packer validate. I had tried to do this once before in the past, but didn't see it through. This commit adds make validate-* targets that make sure everything is syntactically correct once you layer on all of your additional var files. For some builders, this works out of the box, for others it requires setting a few env vars. But the net result is that if you want, you can run the following command to quickly (~6s on my laptop) look for any syntax errors for Packer vars (of which we have encountered many over the life of this project):

 AZURE_LOCATION=fake RESOURCE_GROUP_NAME=fake STORAGE_ACCOUNT_NAME=fake DIGITALOCEAN_ACCESS_TOKEN=fake GCP_PROJECT_ID=fake make -j validate-all

I thought about making a make target that would add these env vars for you, something like make validate-all-test or something similar. I wanted to see what others think. Regardless, I think this would make an excellent CI target as it would catch a large number of errors we end up encountering, without having to need cloud account credentials, and without kicking off a "real" build only to see it fail.

/assign @detiber @CecileRobertMichon
/cc @dvonthenen @EleanorRigby

[k8s-ci-robot]

@codenrhoden: GitHub didn't allow me to request PR reviews from the following users: EleanorRigby.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

This PR has 2 commits.

The first PR fixes an issue seen where the Azure builder is missing the http_proxy variable. This was in the same vein as #335, and I'm still not quite sure why this happened for some users (@dvonthenen) and not others. The previously observed behavior had always been that if you used {{user `some_var`}} and that var hadn't been defined, it ended up as an empty string. The errors that @dvonthenen say indicated otherwise. maybe it was a Packer version or something... Anyways, it has long bothered me that several of these variables were defined in multiple places, so I moved what could be moved into a common location. That's what this commit does. See commit message for a description of what couldn't be moved.

The 2nd commit gives us a way to validate the config using packer validate. I had tried to do this once before in the past, but didn't see it through. This commit adds make validate-* targets that make sure everything is syntactically correct once you layer on all of your additional var files. For some builders, this works out of the box, for others it requires setting a few env vars. But the net result is that if you want, you can run the following command to quickly (~6s on my laptop) look for any syntax errors for Packer vars (of which we have encountered many over the life of this project):

AZURE_LOCATION=fake RESOURCE_GROUP_NAME=fake STORAGE_ACCOUNT_NAME=fake DIGITALOCEAN_ACCESS_TOKEN=fake GCP_PROJECT_ID=fake make -j validate-all

I thought about making a make target that would add these env vars for you, something like make validate-all-test or something similar. I wanted to see what others think. Regardless, I think this would make an excellent CI target as it would catch a large number of errors we end up encountering, without having to need cloud account credentials, and without kicking off a "real" build only to see it fail.

/assign @detiber @CecileRobertMichon
/cc @dvonthenen @EleanorRigby

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: codenrhoden

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• images/capi/OWNERS [codenrhoden]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[codenrhoden]

One interesting point is that if I take these commits, and throw away the first one (that one about the common vars) and run validate-all, it passes. This is what I would have expected (meaning the repo was in good shape), but it doesn't explain the errors that @dvonthenen had been seeing in his own runs. So, @dvonthenen I'll have to work with you to track down the source of the errors you are seeing becuase something else is going on. I still think the change here has merit, because it makes the different vars more explicit, not duplicated, and is a net reduction of LOC.

[codenrhoden]

/hold
Just in case a casual /lgtm comes in on this one. :)

[dvonthenen]

/lgtm

[EleanorRigby]

Just one thing I would like to add is that we can also extract some OS-specific variables to the top level and create ubuntu-args.json and centos-args.json...
Some of these are:

• Distribution
• Version
• build_name
  What do you think? It could be a separate PR

[dvonthenen]

Just one thing I would like to add is that we can also extract some OS-specific variables to the top level and create ubuntu-args.json and centos-args.json...
Some of these are:

• Distribution
• Version
• build_name
  What do you think? It could be a separate PR

If it's possible to do this in a future PR, that would be ideal since I have other time-sensitive dependencies on getting this merged.

[codenrhoden]

Just one thing I would like to add is that we can also extract some OS-specific variables to the top level and create ubuntu-args.json and centos-args.json...
Some of these are:

• Distribution
• Version
• build_name
  What do you think? It could be a separate PR

If it's possible to do this in a future PR, that would be ideal since I have other time-sensitive dependencies on getting this merged.

This is also a bit of a balancing act between what to improve here within the limitations we have given Packer templates, and what we can do with a full-fledged Go CLI and YAML templating. So I'm in favor of holding off on such a thing for now as well.

@dvonthenen as for your dependencies, I'd at least like to get some input from @detiber or @CecileRobertMichon on this one, but the overlap with KubeCon makes that a bit harder. This PR doesn't introduce any user-facing changes, so I'll hold out through today and then consider removing the hold.

[EleanorRigby]

/lgtm

[k8s-ci-robot]

@EleanorRigby: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[codenrhoden]

/hold cancel

[dvonthenen]

/lgtm