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
Add new flags into alpha events #110007
Add new flags into alpha events #110007
Conversation
/cc @bboreham |
Can you add unit tests for the events package? We're trying to improve coverage and I think this is something that would be really helpful here. |
@brianpursley I added unit tests for events package. Could you PTAL?, thanks. |
Would be nice to mention addressing #110622 here and I guess mentioning that in the changelog? |
Good point, let me add this into description. |
@soltysh @brianpursley @eddiezane kindly pinging for reviews whenever you have time. Thanks. |
|
||
// PrintFlags composes common printer flag structs | ||
// used in the Event command. | ||
type PrintFlags struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is worrisome, this is repeating the pattern from get we didn't want to copy anywhere else. get was and still is a snowflake 😞 and I'd prefer we don't repeat that same mistake. IIUC the only reason we need this is the ability to support --output-watch-events
which I'm personally ok not having for this command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
events command has custom printing mechanism unlike to other commands and this printing mechanism includes 3 flags(--output-watch-events
, --no-headers
, --all-namespaces
). What is the preferred way to manage this default event printing by also supporting json and yaml printing?. Because only other command has such requirement is get
command and apparently it is not achieving this in a preferred way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--no-headers
from what I see is already supported in cli-runtime, seeNoHeaders bool --all-namespaces
is a functionality of a command not a printer, it tells to go through all namespaces. Also I'm inclined to skip it forkubectl events
since I don't see much value for it. We can always add it later if needed.--output-watch-events
that one is actually related with printing, but as I mentioned elsewhere, I'd skip it just like--all-namespaces
for now. We can revisit that decision at a later time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kubectl alpha events --all-namespaces
works today; I certainly use it.
--output-watch-events
is more debatable, since kubectl alpha events
deliberately ignores delete events; this was one of the original rationales for having a separate command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed --output-watch-events
and custom print flags altogether. If user specifies output
format, we'll use jsonyaml printing. If user does not specify anything, we'll fallback to current custom event printing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kubectl alpha events --all-namespaces
works today; I certainly use it.
I can be persuaded to include that particular one, like I said, that's definitely not one of the blocker on this.
@ardaguclu can you also re-organize your commits and squash this to some reasonable number, currently I see a lot of commits that could easily be squashed together. |
616e6ea
to
0946e6a
Compare
/test pull-kubernetes-e2e-kind |
0946e6a
to
237e9ce
Compare
237e9ce
to
7291b72
Compare
@soltysh I re-organized commits and did suggested changes as well as new integration tests for new flags. Could you PTAL once again?. Thanks. |
} | ||
} else { | ||
printer = NewEventPrinter(flags.NoHeaders, flags.AllNamespaces) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I now understand where your confusion about the printers is coming from. Your current solution is adding entirely new printer which has to know how to print a resource, which is what we solve in kubectl get
through kubernetes/enhancements#578.
For that mechanism to work, you have to do the following:
- wire in the table printer from here: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/cli-runtime/pkg/printers/tableprinter.go and its flags (this will also give you
--no-headers
for free) - request tabled output (see
kubectl get
:kubernetes/staging/src/k8s.io/kubectl/pkg/cmd/get/get.go
Lines 444 to 448 in 0532324
req.SetHeader("Accept", strings.Join([]string{ fmt.Sprintf("application/json;as=Table;v=%s;g=%s", metav1.SchemeGroupVersion.Version, metav1.GroupName), fmt.Sprintf("application/json;as=Table;v=%s;g=%s", metav1beta1.SchemeGroupVersion.Version, metav1beta1.GroupName), "application/json", }, ",")) kubectl get
does here:kubernetes/staging/src/k8s.io/kubectl/pkg/cmd/get/get.go
Lines 234 to 237 in 0532324
// human readable printers have special conversion rules, so we determine if we're using one. if (len(*o.PrintFlags.OutputFormat) == 0 && len(templateArg) == 0) || *o.PrintFlags.OutputFormat == "wide" { o.IsHumanReadablePrinter = true }
This all can be done in a followup, since that will require a bit more work, and I don't want to block it on that.
The main purpose behind re-using the server-side printing is to ensure we don't have client-side code which knows how to deal with a particular resource, but rather allow the server to tell us how to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll open a followup PR according to the above elaborated description. I was not aware server side printing and indeed that made me confused.
In order to promote kubectl alpha events to beta, it should at least support flags which is already supported by kubectl get events as well as new flags. This PR adds; --output: json|yaml support and does essential refactorings to integrate other printing options easier in the future. --no-headers: kubectl get events can hide headers when this flag is set for default printing. Adds this ability to hide headers also for kubectl alpha events. This flag has no effect when output is json or yaml for both commands. --types: This will be used to filter certain events to be printed and discard others(default behavior is same with --event=Normal,Warning).
7291b72
to
63b8684
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
/kind bug
What this PR does / why we need it:
In order to promote
kubectl alpha events
to beta, it should at least support flags which is already supported bykubectl get events
as well as new flags.This PR is a continuation of initial PR of
kubectl alpha events
and adds some of other flags proposed in https://github.com/kubernetes/enhancements/tree/master/keps/sig-cli/1440-kubectl-events#design-detailsIt adds new flags;
--output
: This PR addsjson|yaml
support and does essential refactorings to integrate other printing options easier in the future like custom-column, etc.--no-headers
:kubectl get events
can hide headers when this flag is set for default printing. This PR adds ability to hide headers also forkubectl alpha events
. This flag has no effect when output is json or yaml for both commands.--types
: This will be used to filter certain events to be printed and discard others(default behavior is same with--event=Normal,Warning
).Which issue(s) this PR fixes:
Fixes #110622
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: