-
Notifications
You must be signed in to change notification settings - Fork 302
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
No, it's german for "The Bootstrap, the" #1958
base: main
Are you sure you want to change the base?
Conversation
69f0510
to
49d5d3f
Compare
Naming bike-shedding: I wonder if Apologies, I haven't looked/thought deeper about the PR more broadly, I only have this bike-shed right now 😅 |
Also: I was totally baited into looking at this by the excellent PR title 🤡 |
@pda i think i agree with you here - it's terser while also holding more information. how would you feel about renaming the command to
my cunning plan has worked then |
e0cd7ca
to
351a916
Compare
351a916
to
303f0d6
Compare
Interesting question. I'm a proponent of ubiquitous language; it'd be a shame to have two names for one thing. One arguable argument against “runner” is that other platforms call their entire agent a “runner” (GitHub Actions, GitLab), and a subset of our customers will confuse it with that. The other that you touched on is that we already have a component called JobRunner which lives in the agent outside the I don't have the answers 🤷♂️ I wonder…
Maybe Taking a step back from specifics…
Through that lens, the subprocess has a much stronger claim to “run job” or “job runner” naming, and the main process should find a different name that means ”knows that a job needs running and knows how to ask a subprocess to run the job”. |
Possible alternative names for
None of those feel great. What does it actually do?
The “k8s/etc” bit means it's not a I'd call it
Maybe it's a The fact that it's learning run jobs in different ways (subprocess / k8s / …) feels important here. Again, “dispatcher” kind of suits that. So does “strategy”. |
@pda very interesting thoughts 🤔 i agree with you that there remains some confusion about the role of the the good thing is that those names ( |
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.
Really pumped for this to happen!
For cleaning up all references to bootstrap, it's probably fine to do this when we delete the bootstrap command. We should also change the bootstrap-script
config key then. For now, we have to keep this config key.
As for the name of the cli command, I prefer subject-object-verb order to subject-verb-object order (despite being an English speaker and vim user). See https://cosine.blue/2019-09-06-kakoune.html, https://simblob.blogspot.com/2019/10/verb-noun-vs-noun-verb.html
So I prefer
buildkite-agent job run
This has the advantage that if we want to add other acions you can perform on a job, we can nest them under the same job
subcommand namespace.
SOV order is also consistent with what we have done for the OIDC. There, the command is buildkite-agent oidc request-token
.
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.
Firstly, bravo for doing this 👏 it's grungy and fiddly work, so you earn A Kudos from me for taking it on. 🎆
I'm pumped for this get this landed! Sorry it's taken me a while to review it, I wanted to give it the review it deserves.
Code looks pretty good! Unfortunately I don't have any particular opinion on the naming.
agent/job_runner.go
Outdated
hookExit := r.preExecHook(ctx, "pre-bootstrap") | ||
hookExit = r.preExecHook(ctx, "pre-exec") |
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.
Is the change in behaviour intended here? The hookExit from pre-bootstrap is overwritten by the pre-exec hookExit. So pre-bootstrap would no longer be able to reject the job.
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.
oops, it totally wasn't! fixed now in 088fa3a
@@ -132,8 +132,9 @@ func (gr *gitRepository) Close() error { | |||
func (gr *gitRepository) Execute(args ...string) (string, error) { | |||
path, err := exec.LookPath("git") | |||
if err != nil { | |||
return "", err | |||
return "", fmt.Errorf("finding git executable on path: %w", err) |
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.
Yes! 😄
d43a835
to
93e996a
Compare
`exec-job` does the exact same thing as bootstrap, it just has a much better name
93e996a
to
421f4fc
Compare
I quite like I'd always imagine that we'd add "Executors" which where strategies for executing the bootstrap, what we do now is a Finding the right name for the bootstrap is a real challenge. The architecture we've built at CashApp where we run the What if you actually decoupled it from the The other aspect here of what the bootstrap does is it manages phases (I wish I'd called this stages), hooks and plugins. I've frequently wanted more granular access to these things, for instance being able to call If I was pushed to pick a name for a straight sub-command rename, I'd actually aim to extract it out of the |
(joke context)
The core of the buildkite agent (one of its cores, anyway) is a component currently called "The Bootstrap". This is the part of the agent that's actually responsible for running jobs, streaming their logs back to the buildkite mothership, and doing all the business of running hooks, finding plugins, doing git things, etc.
Were it only that simple.
What we call "the bootstrap" is actually three separate components from this repo's point of view:
buildkite-agent bootstrap
, which is what the agent calls when it gets a new job to runbootstrap
that contains most of the code that gets run to run a jobbootstrap.Bootstrap
which holds the logic for job execution (though there are other peripheral job execution-related bits and bobs hanging around in thebootstrap
package mentioned above)These three things being named the same thing makes talking about them separately a pain; when talking about "the bootstrap", there's a variety of things that could be the subject of discussion.
Furthermore...
"Bootstrap" is a kind of a crappy name for what this thing does
There was a time, long ago, when this name probably fit. Fun fact, prior to v3 of the agent, the bootstrap used to be a bash script that the agent ran. At this point, the bootstrap was mostly responsible for standing up (bootstrapping, one might say) an environment in which a job (at the time a bash script and nothing more).
Times have changed however, and the bootstrap is now a (very) complex piece of go code responsible for orchestrating all of the various tasks that need to happen before, during and after a job run.
Okay, but why change it?
Simply put, the name is confusing and it means that when we talk about the bootstrap (which we usually mean as "the job execution thingy") to our colleagues and to our customers, there's context that's lost in translation.
The bootstrap is an incredibly important part - maybe the most important part - of a job's execution lifecycle, and we fairly regularly have need to talk to customers about it. Knowing what the bootstrap actually is requires knowledge of the agent's history though, and it makes talking about these things, and intuiting how the agent actually works, a lot harder.
Consider: If you were a buildkite customer and a bikkie said "oh that's a bootstrap error", what would you think the problem is? How about if they said (foreshadowing) "I think there's an error in the job executor"?
Cool. What have you done about it?
This PR is basically a big fancy find-and-replace. The gist of it is:
buildkite-agent bootstrap
command is deprecated (but not removed) and replaced withbuildkite-agent run-job
. This new command is functionally identical to the existing one, with the only change being that it doesn't have a deprecation noticebootstrap
package has been renamed tojob
. This makes a lot of names clearer IMO - considerbootstrap.Shell
vsjob.Shell
boostrap.Bootstrap
struct has been renamed tojob.Executor
. This is more in line with what it actually does - it executes a jobNone of these names are final - i'd love some feedback on them. Two hard things and all that.
Open Questions
job.Executor
too similar semantically toagent.JobRunner
? My opinion is no, but it's not particularly strongly heldStill to do
agent/job_runner.go
to:pre-exec
, identical topre-bootstrap
but with the shiny new namepre-bootstrap
hook??? should we just continue to allow it?bootstrap
. They're pervasive!buildkite-agent exec-job
as its job executor by defaultbuildkite-agent bootstrap
still works okay, but outputs a deprecation warning--bootstrap-script
and--job-executor-script
.