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.

Sign up for GitHub

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

Jump to bottom

Hubble observe flows #875

Merged
merged 1 commit into from
Feb 6, 2023
Merged

Hubble observe flows #875

merged 1 commit into from
Feb 6, 2023

Conversation

chancez
Copy link
Contributor

@chancez chancez commented Jan 26, 2023

Attempt number two for #851 and #543.

Hubble observe is now an alias for hubble observe flows. The only major difference is the help text for hubble observe is now briefer and indicates it is an alias for hubble observe flows. Additionally, while refactoring, I removed the flow formatting flags from the general formatting flagset and just moved it to it's own flagset.

This is currently based on #874 which adds unit tests to ensure things do not break this time.

@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 Jan 26, 2023
@chancez chancez force-pushed the pr/chancez/hubble_cli_unit_tests branch 2 times, most recently from d405618 to c4f8ecb Compare January 26, 2023 17:12
@chancez chancez mentioned this pull request Jan 26, 2023
Merged
@chancez chancez force-pushed the pr/chancez/hubble_cli_unit_tests branch from c4f8ecb to cc9b44b Compare January 26, 2023 17:23
Base automatically changed from pr/chancez/hubble_cli_unit_tests to master January 26, 2023 23:17
@chancez chancez force-pushed the pr/chancez/hubble_observe_alias_attempt2 branch from edc3738 to e4f6118 Compare January 26, 2023 23:19
@kaworu kaworu added the :basecamp: needs-rebase This PR needs to be rebased because it has merge conflicts. label Jan 31, 2023
@chancez
Add hubble observe flows
ef04656 
Hubble observe is now an alias for `hubble observe flows`.
The only major difference is the help text for `hubble observe` is now
briefer and indicates it is an alias for `hubble observe flows`.
Additionally, while refactoring, I removed the flow formatting flags
from the general formatting flagset and just moved it to it's own
flagset.

Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez chancez force-pushed the pr/chancez/hubble_observe_alias_attempt2 branch from e4f6118 to ef04656 Compare January 31, 2023 16:31
@chancez chancez marked this pull request as ready for review February 1, 2023 01:11
@chancez chancez requested a review from a team as a code owner February 1, 2023 01:11
@chancez chancez requested review from kaworu and removed request for a team February 1, 2023 01:11
@chancez chancez removed the :basecamp: needs-rebase This PR needs to be rebased because it has merge conflicts. label Feb 1, 2023
@chancez chancez requested review from rolinh and gandro February 1, 2023 18:11
gandro
gandro reviewed Feb 2, 2023
View reviewed changes
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.

Overall looks fine to me. In local testing, I noticed however that hubble observe foo still is an alias for hubble observe flows. Is that intended?

@gandro gandro added the release-note/minor This PR introduces functionality that users may find relevant to operating Hubble. label Feb 2, 2023
@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 Feb 2, 2023
@chancez
Copy link
Contributor Author

chancez commented Feb 2, 2023

Overall looks fine to me. In local testing, I noticed however that hubble observe foo still is an alias for hubble observe flows. Is that intended?

Hmmm I missed that. Let me take a look

@chancez
Copy link
Contributor Author

chancez commented Feb 4, 2023 •
edited

I think that's expected, because we don't specify the allowed arguments for each command. In the current Hubble CLI you can also do hubble observe foo, as well as: hubble list nodes cats dogs any args.

This is because any command with a Run/RunE can use these as args []string in the cobra.Command.Run closures. The reason it works for hubble observe is because it's got a RunE, and it has sub-commands. If it didn't have a RunE, and only had sub-commands, it would error.

I think we can probably fix this by using cobra.Command.Args, but I believe we could do that for all commands in a follow up PR. What do you think?

gandro
gandro approved these changes Feb 4, 2023
View reviewed changes
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.

Makes sense. Yeah I think we should do that in a follow-up PR. Thanks!

kaworu
kaworu approved these changes Feb 6, 2023
View reviewed changes
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.

Thanks! @chancez

@chancez
Copy link
Contributor Author

chancez commented Feb 6, 2023

Lets try this merge thing again 🤞

@chancez chancez merged commit ae900f4 into master Feb 6, 2023
@chancez chancez deleted the pr/chancez/hubble_observe_alias_attempt2 branch February 6, 2023 17:18
@kaworu kaworu mentioned this pull request Jul 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@kaworu kaworu kaworu approved these changes

@rolinh rolinh Awaiting requested review from rolinh

Assignees
No one assigned
Labels
release-note/minor This PR introduces functionality that users may find relevant to operating Hubble.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@chancez @gandro @kaworu