Support global/project/step env even if buildkite trigger step is used

[bradbarrow]

Overview

This PR addresses what appears to be a regression of behaviour introduced in: #41

Buildkite trigger steps don't support a top level env property, but the new global/project env var injection means that all steps, even trigger steps, will at least always have env: {}, causing them to fail:

`env` is not a valid property on the `trigger` step. Please check the documentation to get a full list of supported properties.

This PR introduces the same env vars to each step but for trigger steps, nests it under build, env:

Input

env:
  SOME: var

steps:
  - trigger: some-pipeline
     command:
       - make something

Output

Before

env:
  SOME: var

steps:
  - trigger: some-pipeline
     env:
       SOME: var
     command:
       - make something

After

env:
  SOME: var

steps:
  - trigger: some-pipeline
     build:
       env:
         SOME: var
     command:
       - make something

It handles the existence / non-existence of build and its nested env property.

Outstanding Questions

Since trigger steps don't support top level env, presumably they also can't support:

env:
  BUILDPIPE_SCOPE: project 

Does this mean trigger steps can't be run / skipped /etc per project?

[RikHeijdens]

Hey @bradbarrow,

Overall this PR seems reasonable to me, but I'm not an expert at Go so I'm going to ask for a second pair of eyes to review this changeset.

As far as your outstanding question is concerned, could you perhaps clarify in what scenarios a per-project trigger step is useful? I believe there are other steps that currently don't support a top-level env field, making it impossible for them to support the BUILDPIPE_SCOPE environment variable (e.g. wait, block and input don't support this either). Unless there is a good use-case I'm not sure if this is something that we'd have to address now.

Note that if we wanted to release this, we would also have to add an entry to the changelog and increment version numbers in a few places (similar to #85).

[TBoshoven]

Thanks for the contribution.

I'm not very familiar with trigger steps, but this change itself looks good to me.

[TBoshoven]

Can you please clarify this TODO?
This will help other people pick this up.

[bradbarrow]

Apologies, I think it's errant. I'll remove.

[TBoshoven]

Please clarify this TODO as well.
It seems like the line adds env to build.

[bradbarrow]

Ditto, errant comments. Apologies, will remove.

[TBoshoven]

It would be helpful to add a short comment describing the transformation that is being done in this block.

[bradbarrow]

Thanks both for the feedback. I'll update this PR shortly.

I've since updated our fork to support BUILDPIPE_SCOPE: project on a trigger step, so that it can be made conditional.

I'll take a look at the other step types you mention and how that scales.

Our use case for a per project trigger step is as follows:

We have 3 projects:

• app
• packages
• docs

They share some steps: lint, test, build etc
They have some that are unique to them: e.g. localisation in app

They have nuanced approaches to deployment:

• packages might be published directly in CI to npm
• docs might be synced to s3
• app triggers a deployment in a separate buildkite pipeline that orchestrates some docker stuff

In the case that someone has only changed the docs - we don't want to trigger a build on the app deploy pipeline

We can try get around this using all kinds of conditionals, but it feels just like any other project-scoped step.

If it were trivial to have a CLI command that triggered it, we'd be fine:

steps:
  - label: trigger-deploy
    env
      BUILDPIPE_SCOPE: project
    command:
      - buildkite-agent trigger app-deploy

But the only clean way to trigger builds is with a trigger step, so it's nice to be able to make them conditional even though nesting it under build > env > BUILDPIPE_SCOPE feels very wrong.

[RikHeijdens]

That's an interesting use-case. That definitely helps to make me understand why it might be useful to have the ability to conditionally trigger pipelines.

If it were trivial to have a CLI command that triggered it, we'd be fine

I wonder if this could be solved through buildkite-agent pipeline upload. I believe this can be invoked multiple times to add steps to an existing running build. I think what you should be able to do is have a step as outlined in your example, but then use something akin to ./script/generate_trigger_step | buildkite-agent pipeline upload where generate_trigger_step is a script that is stored in your repository that outputs the step definition for a trigger.

That way we should have to do fewer gymnastics with regards to supporting top-level env fields for other types of steps, which would cause Buildpipe's supported schema for steps to diverge from what Buildkite natively supports. In principle, I'm not against diverging on this front, but I want to make sure that when we do that it is a well considered change as it may cause confusion.

Either way, happy to consider any concrete proposals.

[xzyfer]

FWIW we've been using this patch for a couple months now. Would love to know the next steps :)

[alabeduarte]

Hi all 👋

I created this issue (#90) without knowing there was already a PR for it. I tested this patch and it is working as I would expect (i.e. I'm able to use Buildkite trigger along with buildpipe features) 🎉

Similar to #84 (comment), are there any plans to have these changes applied?

Cheers 🙏