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

Improve dev-doctor version detection and error reporting #32035

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

joestringer
Copy link
Member

@joestringer joestringer commented Apr 17, 2024

Improve some dev-doctor debugging output and standardize the version check to use the same regex to match versions for various different commands.

Tested locally using make dev-doctor, works well for my environment.

Various commands were specifying custom version regexes for the match.
It's easier to just declare one fairly loose version matcher and then
append it in all the relevant binaryCheck declarations.

Signed-off-by: Joe Stringer <joe@cilium.io>
Previously if the command failed, it would only output something like
"exit status 1" and not include the actual output of the command.
Include the command output to improved debuggability.

Signed-off-by: Joe Stringer <joe@cilium.io>
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Apr 17, 2024
@joestringer
Copy link
Member Author

/test

@joestringer joestringer marked this pull request as ready for review April 18, 2024 02:05
@joestringer joestringer requested a review from a team as a code owner April 18, 2024 02:05
@rolinh
Copy link
Member

rolinh commented Apr 18, 2024

This PR is mainly about adding details for protoc, which is a requirement to run make generate-k8s-api.

Sorry, my comment is a bit unrelated to the PR but based on the above I wanted to ask: why do we require protoc to be installed locally? In my experience, different protoc versions may generate different code. To address this issue when generating the Hubble API, we use a the cilium-builder image that includes protoc (and a set of protoc plugins). Couldn't such approach be used for make generate-k8s-api?

@joestringer
Copy link
Member Author

@rolinh Ah, I didn't know that protoc was sensitive to the specific version being used, thanks for pointing that out. I just found that I didn't have protoc installed and I needed it in order to generate some code. Maybe the solution to that problem is actually to just dockerize. Let me poke around and see if there's some reason we aren't doing that.

@joestringer joestringer marked this pull request as draft April 18, 2024 17:10
@joestringer
Copy link
Member Author

Alright, I spun up #32063 as a replacement for that part. The other cleanups in this PR could be independently useful though, so I'll re-mark this for review just as a cleanup PR.

@joestringer joestringer changed the title Improve dev-doctor, find protoc Improve dev-doctor version detection and error reporting Apr 18, 2024
@joestringer
Copy link
Member Author

/test

@joestringer joestringer marked this pull request as ready for review April 18, 2024 21:52
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 22, 2024
@aditighag aditighag added this pull request to the merge queue Apr 22, 2024
Merged via the queue into main with commit b7e030b Apr 22, 2024
260 of 263 checks passed
@aditighag aditighag deleted the pr/joe/dev-doctor-20240417 branch April 22, 2024 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants