Skip to content
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

BUG FIX: Ensure Build Title Is Correct When Checkout Is Skipped #3024

Merged
merged 2 commits into from
Oct 7, 2024

Conversation

123sarahj123
Copy link
Contributor

@123sarahj123 123sarahj123 commented Oct 4, 2024

Description

Bug: When using skip-checkout plugin, the build title and commit details will match the previously checked out commit details (rather than taking it from the webhook details).

See video below. I trigger a build via webhook (with skip checkout). It starts off with the correct details, but changes to to the details of HEAD

Screen.Recording.2024-10-04.at.1.00.33.PM.mov

Changes

Ensure agent gets correct commit message associated with webhook-triggered build, rather than reading it from git repo's HEAD.

Modified function sendCommitToBuildkite in internal/job/checkout.go to extract the message from e.Commit rather than HEAD

Testing / Reproduction

To reproduce bug

  • Set up a pipeline (or use an existing one).
  • Ensure a webhook triggers the build on push events (when you push to a branch, it triggers a build).
  • Run a build that performs a checkout so there is a working directory with older commit details.
  • Now add a step to your pipeline.yml to use the skip-checkout plugin. For example:
steps:
  - label: "Skip checkout step"
    command: "echo 'Skipping checkout and returning true'"
    plugins:
      - thedyrt/skip-checkout#v0.1.1: ~
  • Push a new commit and trigger the build via the webhook.

  • Observe the build logs. You'll notice the build takes commit details from the old commit (due to skipping checkout) instead of the new one.

  • Tests have run locally (with go test ./...). Buildkite employees may check this if the pipeline has run automatically.

  • Code is formatted (with go fmt ./...)

@123sarahj123 123sarahj123 marked this pull request as draft October 4, 2024 02:53
@123sarahj123 123sarahj123 requested review from moskyb, DrJosh9000 and a team and removed request for moskyb October 4, 2024 03:14
@123sarahj123 123sarahj123 marked this pull request as ready for review October 4, 2024 03:53
…hook-triggered build, rather than reading it from git repo's HEAD
@123sarahj123 123sarahj123 force-pushed the build-title-incorrect-when-checkout-skipped-bug branch from d85ec3d to c3f6a19 Compare October 4, 2024 03:54
@123sarahj123 123sarahj123 requested a review from wolfeidau October 4, 2024 05:12
@123sarahj123 123sarahj123 added Pipelines Escalations 🦕 PRs that are raised from the escalations board bug labels Oct 4, 2024
Copy link
Contributor

@patrobinson patrobinson left a comment

Choose a reason for hiding this comment

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

This seems pretty reasonable

Copy link
Contributor

@DrJosh9000 DrJosh9000 left a comment

Choose a reason for hiding this comment

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

LGTM

I'm happy this does what it says, and having thought about it, I'm happy that making the build title always reflect the commit rather than whatever HEAD is in the repo is a reasonable approach.

@123sarahj123 123sarahj123 merged commit 49ba821 into main Oct 7, 2024
1 check passed
@123sarahj123 123sarahj123 deleted the build-title-incorrect-when-checkout-skipped-bug branch October 7, 2024 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Pipelines Escalations 🦕 PRs that are raised from the escalations board
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants