Skip to content

pull out common pieces of yaml in the buildkite pipeline #976

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

toshok
Copy link

@toshok toshok commented Nov 22, 2023

This was initially motivated by what I saw as a bunch of needless repetition in plugins due to the buildevents work in #943, but I found other things that could be pulled out as well.

Found a pretty big 🤦 in that yaml can't seem to deal with anchors to and merging references of lists. This causes a bunch of pain in both the commands and plugins anchors that I'll call out inline, and it means we can't express plugins/commands in the steps using the syntax that agents/env uses.

Comment on lines +16 to +23
- &aws_sm_plugin
seek-oss/aws-sm#v2.3.1:
region: us-east-2
env:
BUILDEVENT_APIKEY: honeycomb-api-key
BUILDEVENT_BUILDKITE_API_TOKEN: buildkite-api-token-honeycomb-build-events
- &buildevents_plugin
replayio/buildevents#adb8a05: ~
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally I would be able to express this as:

- &buildevents_plugins
  - seek-oss/aws-sm#v2.3.1:
      region: us-east-2
      env:
        BUILDEVENT_APIKEY: honeycomb-api-key
        BUILDEVENT_BUILDKITE_API_TOKEN: buildkite-api-token-honeycomb-build-events
  - replayio/buildevents#adb8a05: ~

and then reference the anchored list below like:

  - &linux_plugins
    plugins:
      - thedyrt/skip-checkout#v0.1.1:
          cd: /home/ubuntu/chromium/src
      <<: *buildevents_plugins

but alas, that isn't valid yaml. instead each plugin needs its own anchor and those get mentioned individually in the os plugins lists.

BUILDEVENT_APIKEY: honeycomb-api-key
BUILDEVENT_BUILDKITE_API_TOKEN: buildkite-api-token-honeycomb-build-events
- replayio/buildevents#adb8a05: ~
- &buckle_build
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: is there a reason not to use buckle everywhere? that would make it so we can reuse the same build command sequence across linux/macos.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the buck2/buckle build anchors are identical except for two lines. in fact except for two words. This is another place where yaml list merging (and string concatenation) would help a bunch. I didn't really want to create anchors for each shared command here, like I did up in plugins.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually we'll be able to use buckle everywhere, but there's currently a bug in the Windows version benbrittain/buckle#21

@toshok
Copy link
Author

toshok commented Nov 22, 2023

the diff here is pretty hard to read. probably easier to look at the resulting file.

Copy link

@jazzdan jazzdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@toshok toshok force-pushed the toshok/pipeline-refactor branch from 4bbbe15 to fb2a388 Compare November 30, 2023 02:17
@toshok
Copy link
Author

toshok commented Nov 30, 2023

rebased off master ☝️

@toshok toshok force-pushed the toshok/pipeline-refactor branch from fb2a388 to 10f2b66 Compare December 4, 2023 21:10
@toshok
Copy link
Author

toshok commented Dec 4, 2023

and one last rebase to make sure nothing's worse off than before, before I land.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants