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

Add new flags into alpha events #110007

Merged
merged 1 commit into from Jul 28, 2022

Conversation

ardaguclu
Copy link
Member

@ardaguclu ardaguclu commented May 12, 2022

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 by kubectl 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-details

It adds new flags;

  • --output: This PR adds json|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 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).

Which issue(s) this PR fixes:

Fixes #110622

Does this PR introduce a user-facing change?

Adds new flags into alpha events such as --output, --types, --no-headers

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[KEP]: https://github.com/kubernetes/enhancements/issues/1440

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 12, 2022
@k8s-ci-robot k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 12, 2022
@ardaguclu
Copy link
Member Author

@brianpursley
Copy link
Member

/cc @bboreham

@brianpursley
Copy link
Member

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.

@ardaguclu ardaguclu changed the title Add new flags into alpha events WIP: Add new flags into alpha events May 31, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 31, 2022
@ardaguclu ardaguclu changed the title WIP: Add new flags into alpha events Add new flags into alpha events Jun 1, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 1, 2022
@ardaguclu
Copy link
Member Author

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.

@invidian
Copy link
Member

Would be nice to mention addressing #110622 here and I guess mentioning that in the changelog?

@ardaguclu
Copy link
Member Author

Would be nice to mention addressing #110622 here and I guess mentioning that in the changelog?

Good point, let me add this into description.

@k8s-ci-robot k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 29, 2022
@ardaguclu
Copy link
Member Author

@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 {
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. --no-headers from what I see is already supported in cli-runtime, see
  2. --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 for kubectl events since I don't see much value for it. We can always add it later if needed.
  3. --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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

staging/src/k8s.io/kubectl/pkg/cmd/events/events.go Outdated Show resolved Hide resolved
staging/src/k8s.io/kubectl/pkg/cmd/events/events.go Outdated Show resolved Hide resolved
staging/src/k8s.io/kubectl/pkg/cmd/events/events.go Outdated Show resolved Hide resolved
@soltysh
Copy link
Contributor

soltysh commented Jul 25, 2022

@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.

@ardaguclu
Copy link
Member Author

/test pull-kubernetes-e2e-kind

@ardaguclu ardaguclu changed the title Add new flags into alpha events WIP: Add new flags into alpha events Jul 26, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 26, 2022
@k8s-ci-robot k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jul 28, 2022
@ardaguclu ardaguclu changed the title WIP: Add new flags into alpha events Add new flags into alpha events Jul 28, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 28, 2022
@ardaguclu
Copy link
Member Author

@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.

@soltysh I re-organized commits and did suggested changes as well as new integration tests for new flags. Could you PTAL once again?. Thanks.

staging/src/k8s.io/kubectl/pkg/cmd/events/events.go Outdated Show resolved Hide resolved
staging/src/k8s.io/kubectl/pkg/cmd/events/events.go Outdated Show resolved Hide resolved
staging/src/k8s.io/kubectl/pkg/cmd/events/events.go Outdated Show resolved Hide resolved
}
} else {
printer = NewEventPrinter(flags.NoHeaders, flags.AllNamespaces)
}
Copy link
Contributor

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:

  1. 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)
  2. request tabled output (see kubectl get:
    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",
    }, ","))
    ) when no output type was specified (just like kubectl get does here:
    // 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.

Copy link
Member Author

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.

staging/src/k8s.io/kubectl/pkg/cmd/events/events.go Outdated Show resolved Hide resolved
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).
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2022
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2022
@k8s-ci-robot k8s-ci-robot merged commit 5856e83 into kubernetes:master Jul 28, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Jul 28, 2022
@ardaguclu ardaguclu deleted the enhancements-alpha-events branch October 25, 2022 13:44
@soltysh soltysh mentioned this pull request Nov 10, 2022
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubectl area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"kubectl alpha events -A -w" panics with "interface conversion: runtime.Object is *v1.Status, not *v1.Event"
7 participants