sourcegraph / sourcegraph-public-snapshot

Code AI platform with Code Search & Cody
https://sourcegraph.com
Other
10.11k stars 1.29k forks source link

Allow users to skip permission errors #24999

Open malomarrec opened 3 years ago

malomarrec commented 3 years ago

Current behavior:

If the spec includes a repository I don't have read access to, the entire preview execution fails. To reproduce:

Expected behavior:

**On src-cli*: If you use --skip-errors, repositories to which you don't have read access are skipped, with a warning about the reason. That does not fail the entire preview, just skips the repository, with a warning. On SSBC:** same behaviour as above by default.

github-actions[bot] commented 3 years ago

Hey, @sourcegraph/batchers (@eseliger @mrnugget @LawnGnome @malomarrec @chrispine @courier-new) - we have been mentioned. Let's take a look.

malomarrec commented 3 years ago

Experienced by https://github.com/sourcegraph/customers/issues/1

eseliger commented 3 years ago

This is not strictly speaking a bug, we scoped this flag to execution errors, so errors that happen inside your specified steps, which cause the container to exit non-zero. I agree this flag doesn't have the best name, though. And still think this is a valid feature request regardless.

malomarrec commented 3 years ago

Should we include this in skip-errors? Or is there a reason to solve it separately? I think the ideal state would be it's treated as all the other errors, with a useful error message that this is because of missing permissions on this repo. WDYT @eseliger ?

eseliger commented 3 years ago

Agree, I think we should extend the scope of --skip-errors.

mrnugget commented 3 years ago

Just leaving a +1 here: makes sense to me.

malomarrec commented 3 years ago

https://docs.google.com/spreadsheets/d/1OEhzdMSlkGOaWyGKwdiAGlirsKKj9EN45Izn7kdKNTg/edit#gid=0&range=A5930

malomarrec commented 3 years ago

https://github.com/sourcegraph/customers/issues/1 +1 from customer

malomarrec commented 3 years ago

Sync discussion:

malomarrec commented 2 years ago

@eseliger is this still relevant for SSBC? Or is it now a src-cli only issue (so only a 0.5 days)?

eseliger commented 2 years ago

Pretty much the same state as above: In SSBC, currently, we skip repos that you can't see in repositoriesMatchingQuery. If a repo is specified using repository: name we still error, because we thought that when you specify it directly, that you are VERY sure you want to run over that repo so just skipping it seems weird. For warnings, we currently don't have a mechanism. Either we fail and have an error message, or we pass and we don't. If we want to add warnings, that's more than 0.5d.

Is that what you are looking for?

malomarrec commented 2 years ago

And when you say fail it means fail on that one workspace, not fail the batch change correct?

eseliger commented 2 years ago

Fail the preview. So you cannot execute.

malomarrec commented 2 years ago

I think we should add warnings in that case indeed

eseliger commented 2 years ago

Is that a requirement for beta?

malomarrec commented 2 years ago

No, but I'll propose adding this to next quarter's roadmap

eseliger commented 2 years ago

sounds good!