feat/bazel: //cmd/{frontend,server} targets that don't include client bundle for backend integration tests

[Strum355]

Currently, all backend integration tests transitively depend on the client bundle. This results in rebuilds of the closure, and a (likely) cache miss on the test, when modifying any client-side files.
Given that the client bundle isnt needed for these tests, we can create targets that don't include the client bundle in its transitive closure, which should in theory improve the cache hit for backend integration tests by not having client side changes invalidate it. This would also be beneficial in local env, to keep frontend rebuilds down

To do this, we still need to create a web.manifest.json file due to some unfortunate requirement on that file existing as part of initializing the sourcegraph instance. For this I just create an empty json file, select this instead of the client bundle target in client/web/dist/BUILD.bazel based on a //:integration_testing_enabled config setting, and creating a go_binary-wrapping bazel rule + macro that automatically applies a transition to set this to true go_binary_nobundle, and using that rule for the specific //cmd/{server,frontend}:{server,frontend}_nobundle binary rules (along with the relevant oci_{image,tarball} etc rules to consume it).

Test plan

• Integration tests in CI still work
• bazel cquery 'kind("js_library", deps(//cmd/frontend:image_nobundle))', bazel cquery 'kind("js_library", deps(//cmd/server:image_nobundle))', ``bazel cquery 'kind("js_library", deps(//testing:backend_integration_test))'` etc all return empty set
• Release test with marker in web bundle to ensure released images contain the web bundle via sg release run test --version 5.4.2 (commenting out other tests for brevity)

[burmudar]

Approved - but I see CI is unhappy

[jhchabran]

Great stuff, comments are only about naming.

Two things:

• nobundle isn't explicit, I think noclientbundle is better
• as we can puts dots in the target names, I like better :no_client_bundle.image_tarball than image_tarball_nobundle

If both you and @burmudar agree that this type of convention is better, we could codify it in the docs.

[jhchabran]

I feel the name here is too generic, on its own it doesn't tell you much. What about graphql_integration_testing_client_bundle ?

[Strum355]

graphql_integration_testing_client_bundle doesnt tell me anything either 😅 I kinda like integration_testing because its generic enough that we can re-use it.
That being said, given we're changing this from bool_flag to bool_setting, this isnt going to be user-facing anyways

[burmudar]

Great stuff, comments are only about naming.

Two things:

* `nobundle` isn't explicit, I think `noclientbundle` is better

* as we can puts dots in the target names, I like better  `:no_client_bundle.image_tarball` than `image_tarball_nobundle`

If both you and @burmudar agree that this type of convention is better, we could codify it in the docs.

Agreed with JH's points

[jhchabran]

Just go a thought, what prevents to end up with a container pushed without the bundle by accident because the flag was enabled?

[Strum355]

Just go a thought, what prevents to end up with a container pushed without the bundle by accident because the flag was enabled?

Good point. I can replace bool_flag with bool_setting to safeguard against building //cmd/frontend:image_tarball with the flag. Cant guard against someone building+pushing //cmd/frontend:image_tarball_nobundle though. In both cases, it takes deliberately invoking something different
https://github.com/bazelbuild/bazel-skylib/blob/main/rules/common_settings.bzl#L91

[jhchabran]

Just go a thought, what prevents to end up with a container pushed without the bundle by accident because the flag was enabled?

Good point. I can replace bool_flag with bool_setting to safeguard against building //cmd/frontend:image_tarball with the flag. Cant guard against someone building+pushing //cmd/frontend:image_tarball_nobundle though. In both cases, it takes deliberately invoking something different https://github.com/bazelbuild/bazel-skylib/blob/main/rules/common_settings.bzl#L91

👍

Building a release test that inspects the image and looks for the bundle as the ultimate safeguard would be great I think. @burmudar wrote something like this to check that we have the migration files, in the end, that's the same thing.

[Strum355]

Building a release test that inspects the image and looks for the bundle as the ultimate safeguard would be great I think. @burmudar wrote something like this to check that we have the migration files, in the end, that's the same thing.

@jhchabran @burmudar pls check https://github.com/sourcegraph/sourcegraph/commit/a876e5a5e255f4333a616421bac01f827bb3eb0c for release test

[jhchabran]

@Strum355 test plan doesn't mention what you did to test the release tests, could you cover it over there as well ?

Also, could you follow https://www.notion.so/sourcegraph/Writing-a-changelog-entry-dd997f411d524caabf0d8d38a24a878c?pvs=4 along the way 😇 ?

[Strum355]

@Strum355 test plan doesn't mention what you did to test the release tests, could you cover it over there as well ?

Also, could you follow https://www.notion.so/sourcegraph/Writing-a-changelog-entry-dd997f411d524caabf0d8d38a24a878c?pvs=4 along the way 😇 ?

LMK if I got the format right 👍

[jhchabran]

@Strum355 I don't see a ## Changelog section, did you forget to add it ? Otherwise, title is 👍