Skip to content

Conversation

@craigfurman
Copy link
Contributor

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
Copy link
Contributor Author

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

@craigfurman craigfurman marked this pull request as ready for review May 14, 2024 11:06
@craigfurman craigfurman requested a review from a team May 14, 2024 11:06
@craigfurman
Copy link
Contributor Author

@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
Copy link
Contributor

@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
Copy link
Contributor Author

Mechanical release?

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

@craigfurman
Copy link
Contributor Author

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
Copy link
Contributor

varungandhi-src commented May 15, 2024

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

@BolajiOlajide
Copy link
Contributor

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

When src-validate fails to connect to a service, name that service in
the error message.
@BolajiOlajide BolajiOlajide force-pushed the validate-conn-err-context branch from c69e755 to ed1e990 Compare May 15, 2024 11:52
To avoid memory leak / timeout.

Fix up depguard config. Fix a formatting error. Leave one linter error
in place for now, it'll be fixed in a subsequent commit, as it's a bit
more complex.
@craigfurman craigfurman force-pushed the validate-conn-err-context branch from a7ae717 to 6a3ff17 Compare May 15, 2024 13:16
@craigfurman
Copy link
Contributor Author

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 🙏

@craigfurman craigfurman merged commit 849335b into main May 15, 2024
@craigfurman craigfurman deleted the validate-conn-err-context branch May 15, 2024 13:23
@varungandhi-src
Copy link
Contributor

@craigfurman fixed here: #1084

Sorry about the lint error

@craigfurman
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants