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

Updates binary name to comply with the go install command resulted name #169

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pantuza
Copy link

@pantuza pantuza commented Nov 17, 2023

This Pull Request has a quite simple contribution.

When you install using go install github.com/grpc-ecosystem/grpc-health-probe@latest
the binary name is actually grpc-health-probe instead of grpc_health_probe as described in the README.md file.

Thus, this pull request simply updates the binary name in the README.md documentation file.

What problem does it solve?
Basically when a developer follows this documentation, they end up trying to run a command that does not
exist in GOPATH. Since the binary name indeed does not exists, eventually, some developer can abort using
the tool simply because it could not install it. Basically the binary is there, but the name is incorrect.

@ahmetb
Copy link
Collaborator

ahmetb commented Nov 17, 2023

It solves one problem but incompatible everywhere else (e.g. docker images, tarball releases). Do you think addressing this discrepancy on line 47 where go install mentioned would help instead?

@ahmetb
Copy link
Collaborator

ahmetb commented Nov 17, 2023

Another solution might be to rename the module in go.mod (to have _).

@pantuza
Copy link
Author

pantuza commented Nov 18, 2023

Yes, indeed it makes more sense to me as well.
I will make sure update this PR by changing the go.mod and we can check if does the trick.

@pantuza
Copy link
Author

pantuza commented Nov 18, 2023

I have tested it by running go install locally and it works by installing a binary with the name grpc_health_probe under my $GOBIN path. Thank you for the suggestion. Please let me know if there is anything else we need to validated/test.

@ahmetb
Copy link
Collaborator

ahmetb commented Nov 18, 2023

I have a feeling like this will break go-get due to repo name mismatch, if not, it'll break users who were installing via go-get. What do you think?

@ahmetb
Copy link
Collaborator

ahmetb commented Nov 27, 2023

Sadly changing the module name will break too many users.
https://sourcegraph.com/search?q=context:global+go+install+github.com/grpc-ecosystem/grpc-health&patternType=standard&sm=1&groupBy=repo
I don't think this is a huge problem. We can just say "if installed via go install the binary name will be placed as grpc-health-probe under $GOBIN directory".

@pantuza
Copy link
Author

pantuza commented Dec 6, 2023

Sadly changing the module name will break too many users. https://sourcegraph.com/search?q=context:global+go+install+github.com/grpc-ecosystem/grpc-health&patternType=standard&sm=1&groupBy=repo I don't think this is a huge problem. We can just say "if installed via go install the binary name will be placed as grpc-health-probe under $GOBIN directory".

Interesting. Yeah, I agree with you.
I will make sure to add such entry on the README so you can review it, if that makes sense.

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.

None yet

2 participants