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] Buildkite did not pick up new env variable. #7403

Merged
merged 4 commits into from
Dec 13, 2023
Merged

[BUG] Buildkite did not pick up new env variable. #7403

merged 4 commits into from
Dec 13, 2023

Conversation

1Copenut
Copy link
Contributor

@1Copenut 1Copenut commented Dec 6, 2023

Summary

The new DEPLOY_ROOT env variable shipped in PR #7397 didn't get picked up in the test run I triggered manually. This PR will be a troubleshooting effort to determine if the variable didn't get passed to the shell script properly, or the catalog-info.yml needs updated.

QA

QA will be done manually, using the Buildkite UI to review environment variables, assert all jobs pass, and the deploy release docs runs correct when PR is merged.

@1Copenut 1Copenut marked this pull request as ready for review December 7, 2023 14:22
@1Copenut 1Copenut requested a review from a team as a code owner December 7, 2023 14:22
@cee-chen
Copy link
Contributor

cee-chen commented Dec 7, 2023

it also might be here:

env:
GIT_BRANCH: "${BUILDKITE_BRANCH}"
GIT_PULL_REQUEST_ID: "${BUILDKITE_PULL_REQUEST}"
GIT_TAG: "${BUILDKITE_TAG}"

when I triggered a manual deploy with custom GIT_TAG env variable, the pipeline_deploy_docs.sh file didn't pick up that env / set it to "". I had to pass BUILDKITE_TAG in order for that to get passed correctly to the second .sh step

@1Copenut
Copy link
Contributor Author

1Copenut commented Dec 7, 2023

it also might be here:

env:
GIT_BRANCH: "${BUILDKITE_BRANCH}"
GIT_PULL_REQUEST_ID: "${BUILDKITE_PULL_REQUEST}"
GIT_TAG: "${BUILDKITE_TAG}"

when I triggered a manual deploy with custom GIT_TAG env variable, the pipeline_deploy_docs.sh file didn't pick up that env / set it to "". I had to pass BUILDKITE_TAG in order for that to get passed correctly to the second .sh step

I think this ^^ is the missing piece of the puzzle. If I add in DEPLOY_ROOT: ${DEPLOY_ROOT}, then it's either undefined or true when I pass it from the Buildkite UI.

On second glance I realize, I forgot to pass this env variable into the Docker env. Which explains why I could echo it in previous tests but the manual job failed. D'oh.

Copy link
Contributor Author

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

Further small refinements, added the env variable to Docker env.

@@ -137,7 +141,7 @@ else

# Our branched docs deploys are **only** ever triggered by tags
# https://buildkite.com/docs/pipelines/environment-variables#BUILDKITE_TAG
elif [[ ! -z "${GIT_TAG}" ]]; then
elif [[ -n "${GIT_TAG}" ]]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to -n on the recommendation from Shellcheck: https://www.shellcheck.net/wiki/SC2236

@@ -103,6 +106,7 @@ if [[ "$1" != "nodocker" ]]; then
--env GIT_PULL_REQUEST_ID \
--env GIT_TAG \
--env CURRENT_RELEASE="${CURRENT_RELEASE}" \
--env DEPLOY_ROOT="${DEPLOY_ROOT}" \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to pass the env variable into Docker. I was able to echo it in previous tests but missed the key detail. This should run correctly now.

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @1Copenut

@1Copenut 1Copenut requested a review from JasonStoltz December 7, 2023 21:50
# Docker images
GCE_IMAGE=google/cloud-sdk:slim
# Test that DEPLOY_ROOT is being passed properly
# TODO: Revert
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this get tested/reverted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It did initially, but I'd like to keep it in the script until I get at least one successful manual run.

Having a quick way to verify the env variable before the build step in the Docker environment cuts my troubleshooting surface in half if it fails again.

Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Let's test it out!

@1Copenut 1Copenut merged commit e489241 into elastic:main Dec 13, 2023
1 check passed
@1Copenut 1Copenut deleted the bug/deploy-docs-manual-override-2 branch December 13, 2023 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants