[CHORE] Refactor Buildkite scripts for better reusability #7358
Conversation
|
I left a few basic comments/questions - reviewing by commit helped a lot! While at a glance the cleanup felt good to me, I don't feel like I have a good enough grasp of buildkite/bash or best practices to fully review this. @tkajtoch, could you take a look here? Or alternatively, Trevor, could we loop in Ops to help review this PR? |
I plan to ping the Ops team as additional reviewers on the next iteration. I'll actually be passing credentials so I can log into Docker via CI to build and push an image. I feel confident with this refactor being correct given yours and Tomasz's votes of confidence. I saw an opportunity to position us for future growth through code reorganization, and learned some best practices that helped a few other folks too. |
|
Ahh gotcha! If there's another PR coming after this, I'm good with rubberstamping/merging it in to unblock you. LMK if that's what you'd prefer! |
| @@ -0,0 +1,7 @@ | |||
| #!/bin/bash | |||
|
|
|||
There was a problem hiding this comment.
[Not a blocking request]
Could you add a comment explaining this script is automatically executed by the agent and link the docs https://buildkite.com/docs/agent/v3/hooks here and in the pre-command script?
There was a problem hiding this comment.
Yes absolutely. I actually had a test failing because main merge didn't update a script location. I'll add the information requested and fix that break. Will merge when tests are green again.
There was a problem hiding this comment.
a README inside hooks/ instead of a comment would also be valid (just IMO!)
There was a problem hiding this comment.
Oh dang, that is a great idea. I'll grab that and add it to the next PR!
|
Preview staging links for this PR:
|
💚 Build Succeeded
History
cc @1Copenut |
Summary
Ops informed me of some improvements I could make while preparing to split our Dockerfile for caching in Buildkite. I took those recommendations and split them into this chore PR. Put simply, this PR moves scripts around, factors our
retryfunction out to a common helper, and removes old Jenkins CI scripts.QA
QA will be done on Buildkite CI. We will know this work was successful if PR jobs for test and deploy docs run successfully.