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

Set delete propagation policy to background when removing jobs and its dependents #71802

Merged
merged 1 commit into from
Dec 11, 2018

Conversation

soltysh
Copy link
Contributor

@soltysh soltysh commented Dec 6, 2018

What type of PR is this?
/kind bug

Which issue(s) this PR fixes:
Fixes #71772

Special notes for your reviewer:
/assign @liggitt @juanvallejo

Does this PR introduce a user-facing change?:

Fixes pod deletion when cleaning old cronjobs

@k8s-ci-robot k8s-ci-robot added do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Dec 6, 2018
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 6, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 approved Indicates a PR has been approved by an approver from all required OWNERS files. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 6, 2018
@soltysh
Copy link
Contributor Author

soltysh commented Dec 6, 2018

/sig apps
/priority critical-urgent

/assign @aleksandra-malinowska @tpepper
for 1.13 approval

@k8s-ci-robot k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Dec 6, 2018
@k8s-ci-robot k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Dec 6, 2018
@juanvallejo
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 6, 2018
@liggitt liggitt added this to the v1.13 milestone Dec 6, 2018
@liggitt
Copy link
Member

liggitt commented Dec 10, 2018

/lgtm

this fixes a regression in the cronjob controller in 1.13

@aleksandra-malinowska
Copy link
Contributor

Is there any reason this can't be an automated cherry-pick of #71801?

@soltysh
Copy link
Contributor Author

soltysh commented Dec 11, 2018

Is there any reason this can't be an automated cherry-pick of #71801?

When I was fixing those I've opened manual PRs for both by cherry-picking PRs. Automatic works only after you have a PR open. Is that a problem?

@soltysh
Copy link
Contributor Author

soltysh commented Dec 11, 2018

Iow. I did git cherry-pick locally.

@aleksandra-malinowska
Copy link
Contributor

@soltysh I'm familiar with making both automated and manual cherry-picks, thanks. I was under the impression that there was a policy for using automated if possible (i.e. unless the PR doesn't go to master at all, or there are conflicts, etc.), but I can't find the source, so maybe it's just that (some) past branch managers required of (some) contributors.

@tpepper LMK if you're OK with skipping the automated cherry-pick process in this case.

@tpepper
Copy link
Member

tpepper commented Dec 11, 2018

When PR'ing directly manually against the branch, there is increased risk that we merge a patch that is different than what ultimately merged to master. After reviewing this one, I'm OK with going ahead with merging as it is in sync.

While the workflow may seem equivalent, or even slightly annoying versus straight manual git for those familiar with that, a very beneficial aspect of the cherry pick automation is that we as patch maintainers automatically know that the patch is the same as what went into master. This is important for us to know as a cohesive project that we don't have slight accidental variations across branches.

I will work to update the documentation to be more clear that manual cherry picks should be exceptional to deal with some emergent situation that requires the action ahead of PR'ing to master.

@aleksandra-malinowska aleksandra-malinowska added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. and removed do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. labels Dec 11, 2018
@liggitt
Copy link
Member

liggitt commented Dec 11, 2018

/retest

@k8s-ci-robot k8s-ci-robot merged commit 4be3fe4 into kubernetes:release-1.13 Dec 11, 2018
@soltysh soltysh deleted the issue71772_1.13 branch January 15, 2019 14:07
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. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants