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

adding the f command to pf extender view #2511

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

wjiec
Copy link
Contributor

@wjiec wjiec commented Jan 29, 2024

In practice, I may be creating port-forward in svc or dp view, but I found that I can't view the enabled forwarding via f command in these views, I have to switch to po view to see it.

So this PR inlines the f command directly in pf_extender, what do you think about this approach?

Copy link
Owner

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@wjiec Nice addition! Thank you for this Jayson!!
The question is do we want to surface the pf indicator on these views to clue users that active pfs are in effect (or not) or leave it as is?

@wjiec
Copy link
Contributor Author

wjiec commented Jan 30, 2024

@derailed Yes, that's a good question, I think it would be clearer to add a pf indicator, and it would also help to indicate the current state between different views. Wouldn't it "break" some backward compatibility to add the PF indicator? What do you think?

@derailed
Copy link
Owner

derailed commented Feb 3, 2024

@wjiec Thanks for your note Jayson!

I think it would be useful to clue users that pfs are active at pod manager level especially in light of these new action.
ie what would prompt you to hit f while in svc or dp in the first place? Seems bubbling this up makes sense.
As for backward compat, the only issue I could foresee is having to change cust view settings to manage this new pf column if users used customization. I don't feel that's a big ask given this affordance.

The stickier point here is to figure out an efficient way to surface status without having to incur an api server call lto find matching pods and hence pfs.
This feels like a heavier lift from this PR original intent.
For this, I think we would need to store pod labels on the pf tracking object and run a match on the viewing resource to assert pf status.

Does this make sense?

@wjiec
Copy link
Contributor Author

wjiec commented Feb 4, 2024

@derailed You are so right! I'm going to try a simple refactoring of the pf tracking object along your lines!

@derailed
Copy link
Owner

derailed commented Feb 4, 2024

@wjiec Thank you Jayson! We could do a follow up PR for this as what you have here is already a good enhancement...

@wjiec
Copy link
Contributor Author

wjiec commented Feb 5, 2024

@derailed Indeed the subsequent optimization is a rather large modification, so let this PR advanced?

@derailed
Copy link
Owner

derailed commented Feb 5, 2024

@wjiec Thank you for looking into it! Figured that might be the case ;(

@derailed derailed merged commit 763a6b0 into derailed:master Feb 5, 2024
3 checks passed
@wjiec wjiec deleted the feat/pf-extender branch February 6, 2024 01:50
@derailed derailed mentioned this pull request Feb 7, 2024
placintaalexandru pushed a commit to placintaalexandru/k9s that referenced this pull request Apr 3, 2024
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.

2 participants