Allow expanding environment variables in image name

[amartani]

Add a new configuration option, expand-image-vars, that, when set to true, evals the image name to expand environment variables.

In particular, my main use case is that we have agents running in multiple AWS regions, and we configure ECR replication such that all our images should always be available on the same region that the agents are running. In this setup, it is desired to specify an image URI from the same region as the agent is running at. However, this is currently not possible to achieve without knowing, beforehand, the region where the agent that will pick up the job is running, as the region needs to be explicitly encoded in the image URI.

This PR makes it possible to do that by allowing the image name to contain environment variables, and hence allows it to reference AWS_DEFAULT_REGION.

[toote]

while I am not against the spirit of the PR (specially because the code looks quite good and complete). I believe that its current form can be too complicated and narrow-minded as the same logic could be applied to a plethora of other registries and would overcomplicate the code of the plugin.

An alternative solution would be to add a single (unsafe) boolean option that would have the image string as specified via de existing plugin configuration go through eval 'echo $image' or something like that allowing for arbitrary variable interpolation at runtime. Such a feature should not only resolve your particular scenario but also any other provider current and future with minimal intervention on the plugin code.

What do you think?

[amartani]

while I am not against the spirit of the PR (specially because the code looks quite good and complete). I believe that its current form can be too complicated and narrow-minded as the same logic could be applied to a plethora of other registries and would overcomplicate the code of the plugin.

An alternative solution would be to add a single (unsafe) boolean option that would have the image string as specified via de existing plugin configuration go through eval 'echo $image' or something like that allowing for arbitrary variable interpolation at runtime. Such a feature should not only resolve your particular scenario but also any other provider current and future with minimal intervention on the plugin code.

What do you think?

Just to be sure, is the suggestion here that I should be able to specify something like:

  image: 123456789012.dkr.ecr.${AWS_DEFAULT_REGION}.amazonaws.com/image:tag

And the plugin should eval the env var?

If it is, I have actually tried that before, and the main problem is that buildkite-agent pipeline upload also evals env vars. So the result is that you end up with the region of the agent that uploaded the steps rather than the region that is running the step, which is quite confusing...

It can be worked around by either using more escaping ($$) or disabling variable interpolation (buildkite-agent pipeline upload --no-interpolation). I think both solutions would be pretty confusing for users, though I am willing to implement it if you believe this would make sense.

[toote]

Just to be sure, is the suggestion here that I should be able to specify something like:

  image: 123456789012.dkr.ecr.${AWS_DEFAULT_REGION}.amazonaws.com/image:tag

And the plugin should eval the env var?

Yup, taking into account that the variable reference needs to be escaped to prevent interpolation when the pipeline is uploaded.

If it is, I have actually tried that before, and the main problem is that buildkite-agent pipeline upload also evals env vars. So the result is that you end up with the region of the agent that uploaded the steps rather than the region that is running the step, which is quite confusing...

I agree it is kind of cumbersome, but it is already being done for volumes and appropriate clarifications are added in the documentation.

It can be worked around by either using more escaping ($$) or disabling variable interpolation (buildkite-agent pipeline upload --no-interpolation). I think both solutions would be pretty confusing for users, though I am willing to implement it if you believe this would make sense.

I believe that it is both a solution to the issue you are presenting as well as something that is not only simpler to implement but also future-proof. The plugin will not need changing every time someones needs a similar support for a new provider or different data that needs changing in the URL itself.

[amartani]

I agree it is kind of cumbersome, but it is already being done for volumes and appropriate clarifications are added in the documentation.

Sounds good! Since this is an established pattern, it makes sense to follow.

PTAL: Re-implemented the PR via a new expand-image-vars config. I copied most of the wording from expand-volume-vars for this; let me know if you want me to format in some different way.

[toote]

great work! just 2 very minor comments :)

[amartani]

Sorry for the delay; I was traveling.

Re-ordered the added entries; note that neither lists are currently fully in alphabetical ordering, but I moved the new entries to where roughly they would be.