{bazci,dev}: send build events to beaver hub#88219
{bazci,dev}: send build events to beaver hub#88219craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
6d72811 to
12501a3
Compare
577041f to
7bcbd00
Compare
7bcbd00 to
e508355
Compare
|
This is now ready for review but shouldn't be merged until beaver hub PR is merged and applied. Only change needed here is updating |
rickystewart
left a comment
There was a problem hiding this comment.
I think the bazci change will conflict with #89845, which will stop bazci from producing a --build_event_binary_file. Can you rebase atop that change and make sure it's still working?
pkg/cmd/dev/build.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| args = append(args, fmt.Sprintf("--build_event_binary_file=%s", filepath.Join(workspace, bepFileBasename))) |
There was a problem hiding this comment.
Hmm, I would rather you just put this in a temporary directory rather than right in the workspace.
There was a problem hiding this comment.
Right but how can we pass the tempdir name back to main where we call sendBepDataToBeaverHubIfNeeded? I see three options:
- keep it like it is
- mutable global variable
- call
sendBepDataToBeaverHubIfNeededat the end ofdev.buildanddev.test, while still not failing or returning any beaver hub errors (just logging them and returning the build/test err if any)
There was a problem hiding this comment.
I don't understand why it's so important the sendBepDataToBeaverHubIfNeeded call is all the way up in main(). Seems like the calls can just be in build() or test(), given those are the only two places where we produce these files.
There was a problem hiding this comment.
So having a temporary dir sounds good to me and I made the change but we have an issue. How can we deal with this dynamic output from testcases?
There was a problem hiding this comment.
I added a --data-driven-test flag, ready for another look now.
577c2b7 to
642ffe4
Compare
cef7a2b to
5175efb
Compare
rickystewart
left a comment
There was a problem hiding this comment.
Instead of adding a new flag, one thing you might try is making a global variable to mark whether this is a test. IIRC we used to have this logic in dev but it was removed due to being no longer needed? Anyway, here's what you can do:
- In some file (e.g.
util.go), addvar isTesting boolas a global variable. This will default tofalsefor all uses indev. - Update to refer to
isTestinginstead ofdataDrivenTestIgnoreTemporaryPaths - In
pkg/cmd/dev/datadriven_test.go, addfunc init() { isTesting = true }. This will ensure that the variable is set only for test builds.
This way you get the behavior during tests and you don't have to update anything unnecessary in the datadriven test files.
Another option is depending on pkg/util/buildutil and checking buildutil.CrdbTestBuild, although you need an extra dependency. It's a tiny dependency so this is probably not a huge deal for dev.
5175efb to
538105d
Compare
Nice, I went with this one. |
pkg/cmd/dev/build.go
Outdated
|
|
||
| func (d *dev) build(cmd *cobra.Command, commandLine []string) error { | ||
| // tmpDir will contain the build event binary file if produced. | ||
| tmpDir, err := os.MkdirTemp("", "") |
There was a problem hiding this comment.
Nit: Consider not creating this directory at all (and not cleaning it up) if buildutil.CrdbTestBuild is set.
|
I think you'll have to update the |
Looks like it is already allowed on master. Edit: oh no it's not nvm.. |
538105d to
d2393a0
Compare
This code change lets `bazci` and `dev` send build events to beaver hub so we can start collecting data about builds in CI and locally. Release note: None Epic: CRDB-8350
d2393a0 to
7f61953
Compare
|
TFTR! bors r=rickystewart |
|
Build succeeded: |
This code change lets
bazcianddevsend build events to beaver hub sowe can start collecting data about builds in CI and locally.
Release note: None
Epic: CRDB-8350