-
Notifications
You must be signed in to change notification settings - Fork 837
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
[INFRA] Updating forked repo logic to build on every branch #6991
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_6991/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two comments for changeset reasoning.
@@ -8,5 +8,5 @@ steps: | |||
build: | |||
env: | |||
GIT_BRANCH: "${BUILDKITE_BRANCH}" | |||
GIT_PULL_REQUEST_ID: "${GITHUB_PR_NUMBER}" | |||
GIT_PULL_REQUEST_ID: "${BUILDKITE_PULL_REQUEST}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The move away from the feature branch target caused Buildkite to disregard GH-specific env variables. Dunno. But I did find this Buildkite default for branches, so updated both instances where I need it to create the comment link.
@@ -10,8 +10,7 @@ | |||
"trigger_comment_regex": "^(?:(?:buildkite\\W+)?(?:build|test)\\W+(?:this|it))", | |||
"always_trigger_comment_regex": "^(?:(?:buildkite\\W+)?(?:build|test)\\W+(?:this|it))", | |||
"set_commit_status": true, | |||
"skip_ci_on_only_changed": ["^.github/", "^generator-eui/", "^wiki/"], | |||
"target_branch": "feature/buildkite-migration" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this line to allow Buildkite (with Elastic org approval still) to kick off jobs against any branch. Kibana does not have this line in the pull-request.json
file either. Feel it's right to remove it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, why was this not caught in #6965?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That branch was built on the upstream
EUI repo, not a fork. I didn't notice it until I made the forked PR this morning to update Cypress.
Wrong branch I was looking at. I didn't put two and two together that #6965 was ignoring Buildkite because of this config change needed. I didn't notice it until I made the forked PR this morning to update Cypress, then talked through the solution with one of the platform engineers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha - I'm more asking about the process used in #6965 that caused this instance to be missed. I would have assumed that searching/grepping for all instances of feature/buildkite-migration
and either changing them to main
or removing the logic completely would have been the approach to take. If it wasn't, I'd be curious why not / what other approach was used that caused this last reference to be missed.
Preview documentation changes for this PR: https://eui.elastic.co/pr_6991/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6991_buildkite/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6991/ |
env: | ||
GIT_BRANCH: "${BUILDKITE_BRANCH}" | ||
GIT_PULL_REQUEST_ID: "${BUILDKITE_PULL_REQUEST}" | ||
BUILDKITE_CI: "${BUILDKITE}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got a Buildkite error saying the step "build" had no type. Read through the docs on passing env variables into command
steps and updated the syntax correctly.
Preview documentation changes for this PR: https://eui.elastic.co/pr_6991_buildkite/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, and I assume CI passing and running correctly proves it works :)
Preview documentation changes for this PR: https://eui.elastic.co/pr_6991/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6991_buildkite/ |
Summary
Testing for Buildkite run against defaultmain
branch. Do not merge.Updating the
pull-request.json
to removetarget_branch
declaration. This should enable the bot to start pull requests for any forked repo, against any branch on EUI, safely.QA
Test will be verified by Buildkite UI.