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 cli help text for service filtering #730

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

ILLIDOM
Copy link
Contributor

@ILLIDOM ILLIDOM commented May 30, 2022

The way the service field in Hubble flows has been implemented so far is that it only works if the destination IP is the service ClusterIP. This makes the following lines from hubble observe --help misleading:

--from-service filter Show all flows originating in the given service ([namespace/]). If namespace is not provided, 'default' is used
--service filter Show all flows related to the given service ([namespace/]). If namespace is not provided, 'default' is used
--to-service filter Show all flows terminating in the given service ([namespace/]). If namespace is not provided, 'default' is used

This patch changes the help message to address the inaccuracy described above.

Fixes: #713

Signed-off-by: Dominique Illi illi.dominique.v@gmail.com

@ILLIDOM ILLIDOM requested review from a team and glibsm and removed request for a team May 30, 2022 16:52
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label PR is blocked until the release note is set label May 30, 2022
@kaworu kaworu added 📄 area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. kind/enhancement This would improve or streamline existing functionality. ⌨️ area/cli Impacts the command line interface of any command in the repository. release-note/misc This PR makes changes that have no direct user impact. labels May 31, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label PR is blocked until the release note is set label May 31, 2022
Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @ILLIDOM!

I have comments, and also there is a typo in the commit message, please s/desrcribed/described/.

cmd/observe/flows.go Outdated Show resolved Hide resolved
cmd/observe/flows.go Outdated Show resolved Hide resolved
cmd/observe/flows.go Outdated Show resolved Hide resolved
@ILLIDOM ILLIDOM requested a review from kaworu May 31, 2022 08:49
@kaworu
Copy link
Member

kaworu commented May 31, 2022

Thank you @ILLIDOM, could you squash the two commit into one please 🙏?

The way the service field in Hubble flows has been implemented so far is that it only works if the destination IP is the service ClusterIP. This makes the following lines from hubble observe --help misleading:

--from-service filter     Show all flows originating in the given service ([namespace/]<svc-name>). If namespace is not provided, 'default' is used
--service filter          Show all flows related to the given service ([namespace/]<svc-name>). If namespace is not provided, 'default' is used
--to-service filter       Show all flows terminating in the given service ([namespace/]<svc-name>). If namespace is not provided, 'default' is used

This patch changes the help message to address the inaccuracy described above.

Fixes: cilium#713

Signed-off-by: Dominique Illi illi.dominique.v@gmail.com
@ILLIDOM ILLIDOM force-pushed the pr/hubble-cli-help-service-filtering branch from 0cb882a to 8e9d612 Compare May 31, 2022 09:57
@ILLIDOM
Copy link
Contributor Author

ILLIDOM commented May 31, 2022

@kaworu I have done a git reset --soft HEAD~2, commited the changes and pushed to the PR branch. I hope that was as you intended.

Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks!

@gandro gandro merged commit 088bdae into cilium:master Jun 2, 2022
@ILLIDOM ILLIDOM deleted the pr/hubble-cli-help-service-filtering branch June 2, 2022 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⌨️ area/cli Impacts the command line interface of any command in the repository. 📄 area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. kind/enhancement This would improve or streamline existing functionality. 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.

hubble CLI help for service filtering is misleading
3 participants