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

KEP-1440: kubectl events #3373

Merged
merged 1 commit into from
Sep 27, 2022
Merged

KEP-1440: kubectl events #3373

merged 1 commit into from
Sep 27, 2022

Conversation

soltysh
Copy link
Contributor

@soltysh soltysh commented Jun 9, 2022

  • One-line PR description: promote kubectl events to beta

  • Issue link: kubectl events #1440

  • Other comments: none

/assign @ardaguclu @eddiezane
for sig-cli review
/assign @wojtek-t
for prr review

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 9, 2022
@soltysh soltysh mentioned this pull request Jun 9, 2022
Closed
12 tasks
@k8s-ci-robot k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Jun 9, 2022
@k8s-ci-robot k8s-ci-robot requested review from KnVerey and seans3 June 9, 2022 12:13
@k8s-ci-robot k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Jun 9, 2022
@soltysh soltysh force-pushed the kubectl_events_beta branch from d81ed3e to 8560bf2 Compare June 9, 2022 12:15
@ardaguclu
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2022
wojtek-t
wojtek-t reviewed Jun 10, 2022
View reviewed changes
Copy link
Member

@wojtek-t wojtek-t left a comment

Choose a reason for hiding this comment

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

The PRR looks fine overall (modulo one nit I added), but the other comment seems to be a blocking one...

keps/sig-cli/1440-kubectl-events/README.md

##### e2e tests

- missing
Copy link
Member

Choose a reason for hiding this comment

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

All of that are a bit worrying.

Can we first fix that and add tests and only then get back to applying the promotion to Beta?

This thread is related:
https://kubernetes.slack.com/archives/C2C40FMNF/p1654710138987519
and we should treat everyone the same way :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's fair, I'll back it out for now. We'll add on tests and stabilization as is, and promote to beta in 1.26

Copy link
Member

Choose a reason for hiding this comment

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

Thank you! (And thanks for understanding)

keps/sig-cli/1440-kubectl-events/README.md Outdated
Longer term, we may want to require automated upgrade/rollback tests, but we
are missing a bunch of machinery and tooling and can't do that now.
-->
Yes, manual tests using old a new versions of kubectl will be performed.
Copy link
Member

Choose a reason for hiding this comment

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

No, manual tests ...

[They weren't so far.]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@soltysh
Copy link
Contributor Author

soltysh commented Jun 13, 2022

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 13, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 11, 2022
@soltysh soltysh force-pushed the kubectl_events_beta branch from 8560bf2 to c864d56 Compare September 21, 2022 15:16
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2022
@soltysh
Copy link
Contributor Author

soltysh commented Sep 21, 2022

/hold cancel
@wojtek-t @ardaguclu this is ready for 1.26 reviews

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2022
wojtek-t
wojtek-t reviewed Sep 22, 2022
View reviewed changes
Copy link
Member

@wojtek-t wojtek-t left a comment

Choose a reason for hiding this comment

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

Just one clarifying question - other than that LGTM from PRR perspective.

keps/sig-cli/1440-kubectl-events/README.md
- [ ] Gather the feedback, which will help improve the command
- [ ] Extend with the new features based on feedback

- [x] Add e2e tests, increase unit coverage.
Copy link
Member

Choose a reason for hiding this comment

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

I don't see e2e tests done?

I'm not going to block on it, because I see the test/cmd/events.sh which are already integration tests, but according to line 155, I don't see those e2e tests yet. Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

You are right @wojtek-t . e2e tests will be handled with this PR kubernetes/kubernetes#111855

Copy link
Member

Choose a reason for hiding this comment

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

Great - thanks!

@soltysh - can you please either merge that PR first or unmark this bit as not yet done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wojtek-t tagged the e2e PR, so we should be go as is.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

And thanks for your patience and understanding in 1.25 :)

@wojtek-t
Copy link
Member

/lgtm
/approve PRR

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh, wojtek-t

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 Sep 27, 2022
@wojtek-t
Copy link
Member

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 27, 2022
@k8s-ci-robot k8s-ci-robot merged commit f84bea8 into kubernetes:master Sep 27, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Sep 27, 2022
@soltysh soltysh deleted the kubectl_events_beta branch September 27, 2022 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ardaguclu ardaguclu ardaguclu left review comments

@wojtek-t wojtek-t wojtek-t left review comments

@KnVerey KnVerey Awaiting requested review from KnVerey

@seans3 seans3 Awaiting requested review from seans3

Assignees

@eddiezane eddiezane

@wojtek-t wojtek-t

@ardaguclu ardaguclu

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.
6 participants
@soltysh @ardaguclu @k8s-triage-robot @wojtek-t @k8s-ci-robot @eddiezane