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

build: improve goreleaser stuff #290

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chenrui333
Copy link

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Reviewed the developer guide on Submitting a Pull Request
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

match with what is in go-test

sudo cp bin/kubectl-rook-ceph /usr/local/bin/kubectl-rook_ceph

@chenrui333 chenrui333 changed the title chore: improve goreleaser step build: improve goreleaser stuff May 9, 2024
Copy link
Collaborator

@subhamkrai subhamkrai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please describe your changes in PR description

main: ./cmd
binary: kubectl-rook-ceph
binary: kubectl-rook_ceph
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
binary: kubectl-rook_ceph
binary: kubectl-rook_ceph

can you explain why this change is required and how it is helping if you use the krew plugin to install the tool above change is not required, IIUC you are consuming the release build image and directly calling the binary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont know which is more desired, that is why I propose this change to check (as I see the discrepancy with the homebrew artifact, Homebrew/homebrew-core#171289)

sounds like kubectl-rook-ceph is preferred over kubectl-rook_ceph?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also cc @travisn for thoughts

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kubectl-rook_ceph is the correct Name of the binary in order to call the plugin with kubectl rook-ceph.

Offical kubectl Docs:
https://kubernetes.io/docs/tasks/extend-kubectl/kubectl-plugins/#names-with-dashes-and-underscores

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doc indicates that it is "possible" to use underscores, but it is not required. Seems like it is more natural to use dashes, as described earlier in the doc here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that dashes in the filename represents a space in the kubectl command.

the command kubectl foo bar baz is invoked by the user, would have the filename of kubectl-foo-bar-baz.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commands work fine in our testing with the dash as in the readme. Are you seeing otherwise?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm currently on vacation, so I'm unable to test anything. I'm just referring to the Kubernetes documentation, and although dashes currently works, it's not guaranteed to work in the future. I recommend using an underscore.

Just my 2 cents.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, when using binary directly(not with krew) kubectl-rook-ceph it doesn't read it to kubectl rook-ceph but using the binary name kubectl- rook_ceph it reads it to kubectl rook-ceph

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