buildkite environment variable substitution in 3.0

[joscha]

I am not sure if there is an easy way to fix this, or even detect a way to fix this, but I wanted to file it anyway so other people will find it when they google. We had a step in our scripts that dynamically generates some inline bash, like this:

bk_gen_dynamic_step_inception() {
  local cmd="$1"
  local agent_role="$2"
  local timeout_minutes="${3:-2}"
  local branches="${4:-}"

  command=$(cat <<BASH
set -eu -o pipefail
        pipeline_file=\$(mktemp --suffix .yaml --tmpdir pipeline.XXXXXXXXXX)
        trap "rm -rf \"\${pipeline_file}\"" EXIT
        ${cmd} >"\${pipeline_file}"
        buildkite-agent pipeline upload "\${pipeline_file}"
        buildkite-agent artifact upload "\${pipeline_file}"
BASH
)

  bk_comment "pipeline upload step for:"
  bk_comment "${cmd}"
  bk_step \
      --label ":pipeline:" \
      --command "${command}" \
      --agent-role "${agent_role}" \
      --timeout "${timeout_minutes}" \
      --branch-filter "${branches}"
}

basically we use this to produce a new step on-demand by amending the pipeline. The YAML produced from this is as follow:

# pipeline upload step for:
# my_command
  - name: ':pipeline:'
    command: |
        set -eu -o pipefail
        pipeline_file=$(mktemp --suffix .yaml --tmpdir pipeline.XXXXXXXXXX)
        trap "rm -rf \"${pipeline_file}\"" EXIT
        my_command >"${pipeline_file}"
        buildkite-agent pipeline upload "${pipeline_file}"
        buildkite-agent artifact upload "${pipeline_file}"
    branches: ''
    timeout_in_minutes: 2
    parallelism: 1
    artifact_paths: ''
    concurrency: ''
    concurrency_group: ''
    skip: false
    agents:
      role: agent_role

on the Buildkite Agent 2, this would work as expected, as $pipeline_file would be resolved at runtime of the step. But for Agent 3.0 the $pipeline_file would be resolved at the time of generating the step (e.g. before buildkite-agent upload), hence the generated bash would be incorrect and fail the step at runtime.

I wonder if undefined env variables (what $pipeline_file would be at the time of step generation) on inline-commands is really expected? I can obviously add another \, but I think my expectation would be for interpolation only to happen on non-heredoc-style yaml.

[lox]

Sounds like you just need an extra level of escaping on ${pipeline_file} in the bash you are generating? Otherwise they will be interpolated on pipeline upload.

One other thing we've discussed is a --no-interpolation parameter to the pipeline upload command, would that be something you'd use?

[joscha]

One other thing we've discussed is a --no-interpolation parameter to the pipeline upload command, would that be something you'd use?

I think I'd prefer that instead of littering \\ throughout the generated bash. But having said that, we don't have a lot of inline-bash anyway thankfully, so I am happy to add some more \\ to the output. The problem is more that I can only do this once our agents are all on 3.0. We are gradually switching them over, so there is no "right way™" right now.

[lox]

Might there be some other way to solve this problem than generating an inline bash snippet? What problem is being solved with it? Could it be replaced with a plugin? https://buildkite.com/docs/agent/v3/plugins

[joscha]

What problem is being solved with it?

Glad you are asking :) So the problem this solves actually lies with Buildkites notion of concurrency.

We have a saucelabs plan which allows a fixed limit of concurrent connections. Let's say 2. So we set

concurrency: 2
concurrency_group: saucelabs

which theoretically works fine by itself, but means that because of:

Concurrency groups guarantee that jobs will be run in the order that they were created in.

(see https://buildkite.com/docs/builds/controlling-concurrency)

it means that if a long pipeline (let's say 30 minutes of steps) has a saucelabs step at the end, a resource (of which we only have two) is reserved before it's actually needed. For that reason we only put a generation step in, which then (after 30 minutes) when we actually need our resource, amends the pipeline and uploads the step to be executed. Makes sense or is my explanation confusing? So in short, a concurrency_guarantee_order: false would fix it as well for us and actually make our pipeline scripts much simpler.

[lox]

Might a good solution for sauce labs be to have a dedicated pipeline for it and then use synchronous trigger steps for it? Then you could enforce concurrency in that pipeline and also get really good visibility into the pipeline of work for saucelabs testing.

[joscha]

Possible, we would need to hand off some artifacts into that pipeline however. And we use saucelabs from different projects, which are not all uniform (some use webdriverio, some karma) I'll give it some more thought, but not sure it is that simple.

[lox]

Yeah, across multiple pipelines makes it hard. You would need to have a {project}-saucelabs for each project. It's still fairly viable, IMO. I think having a "sidecar pipeline" for things like this makes lots of sense. We do it for release stages in agent and pass through env, params and artifacts:

https://github.com/buildkite/agent/blob/master/.buildkite/steps/upload-release-steps.sh

[lox]

Otherwise, a plugin that generates and uploads the saucelab steps would keep it nicely encapsulated with the approach you have currently.

[joscha]

--no-interpolation would be good I think @lox, we just had a build failure because of a commit message:

fix $ escape, refs #21154

which failed the build due to:

- trigger: 'bla'
--
  | label: ':rocket: Deploy'
  | async: true
  | branches: 'master'
  | build:
  | commit: 'c0ffee'
  | branch: 'green'
  | message: >
  | fix $ escape, refs #21154
  | env:
  | 2018-04-18 07:37:13 DEBUG  Debug mode enabled
  | 2018-04-18 07:37:13 INFO   Reading pipeline config from "/tmp/tmp.AUPulGzo37"
  | 2018-04-18 07:37:13 FATAL  Pipeline parsing of "tmp.AUPulGzo37" failed (Expected identifier to start with a letter, got  )

[lox]

Follow https://github.com/buildkite/agent/issues/715 for that one @joscha!

[toolmantim]

https://github.com/buildkite/agent/pull/733 is now in master and will go out in the next release. Does this solve your issue @joscha?

[joscha]

Awesome @toolmantim - I think so, yes, will close this issue.