sourcegraph / src-cli

Sourcegraph CLI
https://sourcegraph.com
Apache License 2.0
267 stars 57 forks source link

validate: add context to connection error #1081

Closed craigfurman closed 1 month ago

craigfurman commented 1 month ago

When src-validate fails to connect to a service, name that service in the error message.

Test plan

There are no current automated tests for this error message, and this commit does not add any, admittedly. This is arguably only a behavioral change if an external tool is parsing this particular error message.

craigfurman commented 1 month ago

At a glance, the failing build steps do not look relevant to my change, but this is my first contribution to this repo 🤔

craigfurman commented 1 month ago

@BolajiOlajide it looks like these same checks are failing on main so I guess I'm probably ok to merge. This change on its own is probably not enough to cut a release for, but in general, do you know what sort of cadence this tool releases on?

BolajiOlajide commented 1 month ago

@BolajiOlajide it looks like these same checks are failing on main so I guess I'm probably ok to merge. This change on its own is probably not enough to cut a release for, but in general, do you know what sort of cadence this tool releases on?

The cadence is at the discretion of whoever is releasing it. There hasn't been much changes to it in a while , but it deserves a mechanical 5.4.0 release actually.

Can you leave this PR open, I'd love to try to see why these operations are being canceled, to be certain it's not a symptom of something else?

craigfurman commented 1 month ago

Mechanical release?

Sure I can leave it open, will build from source for now

craigfurman commented 1 month ago

It's possible most actions are being cancelled to save resources after scip-go fails. I think there were similar fixes for that committed to sg/sg and other repos recently @BolajiOlajide

varungandhi-src commented 1 month ago

I've landed a workaround for the data race on main and opened a PR for the scip-go failure here: https://github.com/sourcegraph/src-cli/pull/1083

BolajiOlajide commented 1 month ago

I've landed a workaround for the data race on main and opened a PR for the scip-go failure here: #1083

Thank you @varungandhi-src

craigfurman commented 1 month ago

We've fixed the memory-leak / timeout by bumping golangci-lint. We also updated the depguard config. This revealed a few linter errors that are present on main but had been masked by this timeout. We fixed one (a format). @varungandhi-src , see https://github.com/sourcegraph/src-cli/actions/runs/9096390518/job/25001689748?pr=1081 for the other one. I think that's a real issue - we overwrite the passed-in err with the result of ParsePersonalAccessToken() - which could be nil if there is no issue parsing the access token!

We hesitated to fix it because it's a bit complicated, figured we'd flag it to you as the author 🙏

varungandhi-src commented 1 month ago

@craigfurman fixed here: https://github.com/sourcegraph/src-cli/pull/1084

Sorry about the lint error

craigfurman commented 1 month ago

No worries! It would have been hard to spot due to being masked by the golangci-lint timeouts 😁