Skip to content

[CI] Use the same commit from target branch for all steps in post-checkout hook#10397

Merged
mrodm merged 8 commits into
elastic:mainfrom
mrodm:use-same-main-commit-steps
Jul 10, 2024
Merged

[CI] Use the same commit from target branch for all steps in post-checkout hook#10397
mrodm merged 8 commits into
elastic:mainfrom
mrodm:use-same-main-commit-steps

Conversation

@mrodm
Copy link
Copy Markdown
Collaborator

@mrodm mrodm commented Jul 5, 2024

Proposed commit message

Use the same commit (SHA changeset) from the target branch, e.g. main branch, for all steps run in the Buildkite pipeline in the context of Pull Requests. This commit from main will be used in the post-checkout hook when available. Using the same changeset from the target branch in all steps avoids that some steps could be using a newer commit from the target branch if a new PR is merged during the build is running. That would create a different merge branch in the post-checkout hook with different contents.

For instance in this build, sonar step runs a different changeset than previous steps:
https://buildkite.com/elastic/integrations/builds/13288

Looking into that build, it can be observed how two different steps are using different changesets from main (there was a PR merged while the PR was being tested) to generate the temporal branch in the post-checkout hook:

  • Trigger integrations step:
HEAD is now at 0c17c7a80 all: harmonise http status error return behaviour (#10346)
  • Sonarqube step (this step runs after all packages have been tested):
HEAD is now at 185d53021 [AWS] Migrate AWS package to ecs@mappings (#10223)

Related issues

@mrodm mrodm self-assigned this Jul 5, 2024
@mrodm mrodm force-pushed the use-same-main-commit-steps branch from 271d573 to 03eb6e3 Compare July 5, 2024 11:41
@mrodm mrodm force-pushed the use-same-main-commit-steps branch from 03eb6e3 to 06aea19 Compare July 5, 2024 11:45
@mrodm mrodm requested a review from dliappis July 5, 2024 12:29
@mrodm mrodm marked this pull request as ready for review July 5, 2024 12:29
@mrodm mrodm requested a review from a team as a code owner July 5, 2024 12:29
Copy link
Copy Markdown

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

Make sense. Left a couple of suggestions.

Comment thread .buildkite/hooks/pre-command Outdated
git fetch -v origin "${target_branch}"
git checkout FETCH_HEAD
echo "Current branch: $(git rev-parse --abbrev-ref HEAD)"
if [[ ${REPOSITORY_TARGET_BRANCH_COMMIT} == "" ]]; then
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As discussed offline I'd suggest that we add a big fat warning at the top of this file like

# ******************************* WARNING *******************************
# This post-checkout hook is not the same as in the rest of repos e.g. beats
# because some steps in this pipeline (in PR context) take a very long time
# and we want to make sure that THE SAME COMMIT FROM TARGET BRANCH
# gets merged in every pipeline step. Otherwise, HEAD may have changed in the meantime
# and therefore, some steps (e.g. sonarqube) may end up testing a different commit
# *************************************************************************

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done in ccd0969

Co-authored-by: Dimitrios Liappis <dimitrios.liappis@gmail.com>
@mrodm mrodm changed the title [CI] Use same main commit for all steps in post-checkout [CI] Use the same commit from target branch for all steps in post-checkout hook Jul 5, 2024
@elasticmachine
Copy link
Copy Markdown

elasticmachine commented Jul 5, 2024

@elasticmachine
Copy link
Copy Markdown

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@elastic-sonarqube
Copy link
Copy Markdown

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

Copy link
Copy Markdown

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

👍

@mrodm mrodm merged commit 9a2e8df into elastic:main Jul 10, 2024
@mrodm mrodm deleted the use-same-main-commit-steps branch July 10, 2024 16:51
@mrodm mrodm mentioned this pull request May 14, 2026
5 tasks
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.

4 participants