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

Update "KEP-4368 Job API managed-by mechanism" targeting Beta in 1.32 #4856

Merged
merged 3 commits into from
Sep 24, 2024
Merged

Update "KEP-4368 Job API managed-by mechanism" targeting Beta in 1.32 #4856

merged 3 commits into from
Sep 24, 2024

Conversation

mimowo
Copy link
Contributor

@mimowo mimowo commented Sep 16, 2024 •
edited
Loading

One-line PR description: Update the KEP to prepare for update to Beta in 1.32

Issue link: #4368

Other comments:

  • Update the kep.yaml targeting Beta in 1.32
  • Update the check box list
  • Update the decision to make the field immutable
  • Added a point to re-visit the decision wrt skip to enqueue
  • Update the integration tests with links
  • Update the implementation history
  • Typo fixes

Graduation criteria:

@k8s-ci-robot k8s-ci-robot added 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 sig/apps Categorizes an issue or PR as relevant to SIG Apps. labels Sep 16, 2024
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 16, 2024
@mimowo
Copy link
Contributor Author

mimowo commented Sep 16, 2024

/cc @alculquicondor @atiratree @tenzen-y

@mimowo mimowo mentioned this pull request Sep 16, 2024
Open
12 tasks
@mimowo
Copy link
Contributor Author

mimowo commented Sep 16, 2024

/cc @wojtek-t

@mimowo mimowo mentioned this pull request Sep 16, 2024
Merged
3 tasks
@wojtek-t wojtek-t self-assigned this Sep 19, 2024
@mimowo
Copy link
Contributor Author

mimowo commented Sep 20, 2024

/cc @soltysh

wojtek-t
wojtek-t reviewed Sep 23, 2024
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.

Two small comments from PRR perspective - other than that LGTM.

keps/prod-readiness/sig-apps/4368.yaml Show resolved Hide resolved
@mimowo
Copy link
Contributor Author

mimowo commented Sep 23, 2024

/cc @tenzen-y @andreyvelich @ahg-g @danielvegamyhre
To ping maintainers of the other batch projects adopting the managedBy field.

@k8s-ci-robot
Copy link
Contributor

@mimowo: GitHub didn't allow me to request PR reviews from the following users: andreyvelich.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @tenzen-y @andreyvelich @ahg-g @danielvegamyhre
To ping maintainers of the other batch projects adopting the managedBy field.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@wojtek-t
Copy link
Member

/approve PRR

tenzen-y
tenzen-y reviewed Sep 23, 2024
View reviewed changes
Copy link
Member

@tenzen-y tenzen-y 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 the update!
lgtm

keps/sig-apps/4368-support-managed-by-for-batch-jobs/README.md
@@ -1100,6 +1110,26 @@ Why should this KEP _not_ be implemented?

## Alternatives

### Skip reconciliation in the event handler
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for reflecting on our previous discussions here.

@danielvegamyhre
Copy link
Member

Thanks for updating us on this @mimowo. LGTM

andreyvelich
andreyvelich reviewed Sep 23, 2024
View reviewed changes
Copy link
Contributor

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

Thank you for driving this @mimowo!
Just a small nit.

keps/sig-apps/4368-support-managed-by-for-batch-jobs/README.md Outdated Show resolved Hide resolved
soltysh
soltysh approved these changes Sep 24, 2024
View reviewed changes
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

/label tide/merge-method-squash

keps/sig-apps/4368-support-managed-by-for-batch-jobs/README.md
These projects for now follow the decision taken in the core k8s to make the
field immutable to avoid complication of the support for mutability.

All together, we decide to keep the field immutable.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 24, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 24, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mimowo, 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 24, 2024
@k8s-ci-robot k8s-ci-robot merged commit e6475c7 into kubernetes:master Sep 24, 2024
4 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.32 milestone Sep 24, 2024
@mimowo mimowo mentioned this pull request Sep 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tenzen-y tenzen-y tenzen-y left review comments

@andreyvelich andreyvelich andreyvelich left review comments

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

@soltysh soltysh soltysh approved these changes

@kow3ns kow3ns Awaiting requested review from kow3ns

@alculquicondor alculquicondor Awaiting requested review from alculquicondor

@atiratree atiratree Awaiting requested review from atiratree

@danielvegamyhre danielvegamyhre Awaiting requested review from danielvegamyhre

@ahg-g ahg-g Awaiting requested review from ahg-g

Assignees

@soltysh soltysh

@wojtek-t wojtek-t

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/apps Categorizes an issue or PR as relevant to SIG Apps. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
SIG Apps
Archived in project
Milestone
v1.32
Development

Successfully merging this pull request may close these issues.
7 participants
@mimowo @k8s-ci-robot @wojtek-t @danielvegamyhre @soltysh @andreyvelich @tenzen-y