batches: Workspace file upload for local runs

[Piszmog]

Closes #42885.

@BolajiOlajide caught this when working on #31791. Luckily it was a quick implementation.

Test plan

No tests. Did functional testing.

Screen.Recording.2022-10-12.at.10.48.18.mov

[eseliger]

I think I am missing a bit of context, what do we require these files for on the backend?

[BolajiOlajide]

I think I am missing a bit of context, what do we require these files for on the backend?

We need to save them in the database so it's displayable in the UI. Currently only src batch remote ... saves the file in the database.

[eseliger]

Ah! Do we want to show them for local src-cli runs? Feels like a bit of overhead in the case that we don't end up using it, and it might be a surprising change if you used to use secret, local files that now end up being silently stored on the server.

That said, I don't feel strongly about this, but wanted to raise it.
One slight concern is that failing uploads (also due to max file size exceeded) will fail the execution, which would otherwise be perfectly good and is ready to be previewed. Should we just warn, and live with them missing from the UI?

[Piszmog]

I think it makes sense to show them for local runs. That way there is parity with server-side (and moving to server-side will have the same look and feel as local with respect to workspace files).

With regards to failing uploads, I could see warning on file uploads just for preview. Though, since a user can apply a previewed spec via the UI, it would be good to have an alert or something to communicate that not all workspace files made it up.

If a user runs apply I think it should fail (to ensure the UI experience is complete and gets the user use to file limits)

[eseliger]

preview is applicable from the UI as well, so probably they should behave similarly?

[Piszmog]

That's true. Yea they should behave the same. So guess we are ok with this current implementation or need another opinion?

[eseliger]

I'll tag @malomarrec and @danielmarquespt for some further comments while we figure something out that just occurred to me:

We have to do feature detection here. Likely also for the batch remote command where we already added this. This may only be called if the backend is 4.1 or later. src-cli usually is "compatible with all", in that it wouldn't break with other versions.
Don't worry it's not as bad as it sounds, we've done that many times and just recently dropped a lot of flags for 4.0: https://github.com/sourcegraph/src-cli/pull/822/files#diff-eb73e64d9518d33b94d2ce1a93bc7ca28fc4d66c4db8f673d194da4c2dfc0067R11

[malomarrec]

My take: this is a very valuable addition, and will make batch changes more reproducible as a whole (as someone who reads someone else's batch spec, I also get the scripts without which the spec is kinda meaningless.)

As Erik pointed out, we don't really know how folks use it. So I'd recommend being loud about this change. It's actually an additive breaking change . So in practice:

• add to changelog
• add to in-product changelog a message like "Scripts mounted with src-cli will now be uploaded and shown in the UI. You shouldn't hardcode secrets in scripts, use env instead.

Overall, all of this is gonna be much smoother when we support secrets https://github.com/sourcegraph/sourcegraph/issues/27926

[Piszmog]

@malomarrec do you think that local runs should always should be successful even if there is an issue uploading files to the database?

So we have successfully ran locally, we generated all the changesets, and we start uploading everything to Sourcegraph. While uploading the workspace files, turns out a file used to generate the changesets exceeds our limit (10MB). Do you think we should reject the entire run or have something in the UI to communicate that a file did not make it up?

[malomarrec]

I think a warning in the UI is much better: what failed is not blocking the batch change, so it shouldn't fail

[Piszmog]

Per @danielmarquespt

Ok, so I think we should only display anything when it fails to be on the UI. Otherwise I’m afraid we might worry the user with a warning message that has no impact for them.

We can create the logic to prepare the UI work, because then we need to show it failed when UI displays the mounted files, but right now, if it hasnt any impact then I dont think we should show anything

[Piszmog]

I'll update src to log a warn but nothing will show in the UI (as part of this Issue)

[danielmarquespt]

Regarding the choice of not displaying the notice:

AFAIK right now, a file failing to upload has no impact on anything. So I think we shouldn't display a warning and worry the user that something failed, when it has no impact.

In the future, when we display the files in the UI then we need to disclose that something failed.

[Piszmog]

I have update the UI to log a warning. But the error does not stop the execution.

[LawnGnome]

I'm probably -0 on the actual change (I think the downsides of uploading local, potentially private files outweigh the advantages here), but since everyone else is fine with it, LGTM technically.