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

Honor global namespace flag in sysdump command #1302

Merged
merged 2 commits into from
Jan 24, 2023
Merged

Honor global namespace flag in sysdump command #1302

merged 2 commits into from
Jan 24, 2023

Conversation

ChrsMark
Copy link
Contributor

As reported at #1079 the global --namespace flag is not honored in sysdump command.

This PR honors the global --namespace flag in sysdump commad by making --namespace flag equivalent to the --cilium-namespace flag.

If both flags are set, then the --namespace global flag comes with priority.

Closes #1079

Signed-off-by: ChrsMark [email protected]

@ChrsMark ChrsMark requested a review from a team as a code owner December 18, 2022 12:57
@ChrsMark ChrsMark requested a review from joamaki December 18, 2022 12:57
@ChrsMark ChrsMark temporarily deployed to ci December 18, 2022 12:59 — with GitHub Actions Inactive
@ChrsMark ChrsMark force-pushed the fix_sysdump_ns_flag branch from 875113a to e163dba Compare December 18, 2022 13:12
@ChrsMark ChrsMark temporarily deployed to ci December 18, 2022 13:12 — with GitHub Actions Inactive
@ChrsMark ChrsMark force-pushed the fix_sysdump_ns_flag branch from e163dba to fd137eb Compare December 19, 2022 20:35
@ChrsMark ChrsMark temporarily deployed to ci December 19, 2022 20:35 — with GitHub Actions Inactive
@ChrsMark
Copy link
Contributor Author

Hey @joamaki! Any ideas if the failing test is related? I cannot understand a lot of things from the output 😞

@ChrsMark ChrsMark mentioned this pull request Dec 21, 2022
Closed
tklauser
tklauser requested changes Dec 21, 2022
View reviewed changes
internal/cli/cmd/sysdump.go Outdated Show resolved Hide resolved
@ChrsMark ChrsMark temporarily deployed to ci December 21, 2022 14:32 — with GitHub Actions Inactive
@ChrsMark ChrsMark temporarily deployed to ci December 22, 2022 10:11 — with GitHub Actions Inactive
@ChrsMark ChrsMark requested review from tklauser and removed request for joamaki December 22, 2022 10:12
@ChrsMark
Copy link
Contributor Author

Hey @tklauser, I tried to address the comment(s) here so feel free to have another look when you get the chance.

tklauser
tklauser requested changes Jan 10, 2023
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Thanks for your patience @ChrsMark. I think this is still not completely correct as it would now break the namespace auto-detection, please see my comment inline.

internal/cli/cmd/sysdump.go Show resolved Hide resolved
@ChrsMark ChrsMark temporarily deployed to ci January 19, 2023 10:33 — with GitHub Actions Inactive
@ChrsMark
Copy link
Contributor Author

Hey @tklauser and thanks for the review again! I agree, what your proposed here looks more robust.
Summarizing on my own benefit with some examples:

  1. ./cilium --namespace foo sysdump --cilium-namespace bar -> namespace value will be bar
  2. ./cilium sysdump --cilium-namespace bar -> namespace value will be bar
  3. ./cilium --namespace foo sysdump -> namespace value will be foo
  4. ./cilium sysdump -> nothing will be set and the auto-detection logic will take place.

Based on this, feel free to have another look and let me know what you think :).

@ChrsMark ChrsMark requested a review from tklauser January 24, 2023 11:05
tklauser
tklauser approved these changes Jan 24, 2023
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks a lot @ChrsMark.

ChrsMark and others added 2 commits January 24, 2023 14:07
@ChrsMark @tklauser
Honor global namespace flag in sysdump command
b269675 
As reported at cilium#1079 the global --namespace flag is not honored in
sysdump command.

This PR honors the global --namespace flag in sysdump commad by making
--namespace flag equivalent to the --cilium-namespace flag.

If both flags are set, then the --namespace global flag comes with
priority.

Closes cilium#1079

Signed-off-by: ChrsMark <[email protected]>
@tklauser
sysdump: always log used namespaces
6a4411b 
Also log the namespaces in case they were specified on the command line.

Signed-off-by: Tobias Klauser <[email protected]>
@tklauser tklauser force-pushed the fix_sysdump_ns_flag branch from c9ed8c1 to 6a4411b Compare January 24, 2023 13:07
@tklauser tklauser temporarily deployed to ci January 24, 2023 13:07 — with GitHub Actions Inactive
@tklauser tklauser merged commit 8f1d0aa into cilium:master Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tklauser tklauser tklauser approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: cilium sysdump predates global --namespace flag
2 participants
@ChrsMark @tklauser