Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

validate: add context to connection error #1081

Merged
merged 2 commits into from
May 15, 2024
Merged

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 as a code owner 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.
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
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
7 of 8 checks passed
@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.

3 participants