Welcome @sweeneyb!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

Hi @sweeneyb. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sweeneyb
To complete the pull request process, please assign mengqiy
You can assign the PR to them by writing /assign @mengqiy in a comment when ready.

The full list of commands accepted by this bot can be found here.

/assign @mengqiy

/ok-to-test

/retest

@sweeneyb Looks like some of the failures are unrelated to this PR. I'm having a look at what is going on.

@cblecker thanks. I'll defer retests until tomorrow or I hear something. I can see one test dying being spurious. But it's failed a couple of times now. I don't want to put load on the CI unnecessarily.

/cc @apelisse @jennybuckley

/retest

The above failure is legitimate. You need to run hack/update-gofmt.sh

/retest

/retest

@sweeneyb: The following test failed, say /retest to rerun all failed tests:

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

All required tests are passing.

The IPv6 test is soft-blocking, and seems to be having issues unrelated to this PR.

tide Pending — Not mergeable. Needs approved, lgtm labels.

this status is also the relevant one for knowing what is overall required (it could be more obvious...), this PR won't merge until approved and lgtm-ed, but it's already good on the tests.

tide is the kubernetes merge robot.

@BenTheElder Thanks for the context. It is pretty obvious in retrospect. There's a lot new, here, so I'm learning where to look in a lot of ways. Thanks again.

FWIW I think it's a lot and not anywhere near as obvious as it should be.

This PR changes the behavior of delete directive$patch: delete.
Before: it deletes all entries that match the mergeKey.
After: it deletes the first entry that matches the mergeKey.

The later approach doesn't solve all of the problems.
Suppose user has a list like this:

- name: foo
  anotherField: abc
- name: foo
  anotherField: xyz

If the user wants to remove

- name: foo
  anotherField: xyz

there is no great way to express that.
If using SMP, the user needs to delete the 1st entry, delete the 2nd entry and then add the 1st entry back.

I'm not sure if the new behavior is better than the existing behavior.
@lavalamp @liggitt @apelisse WDYT?

Some background:
@lavalamp suggested we should remove all matching entries: kubernetes/community#140 (comment)
It is implemented in #38342

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@fejta-bot: Closed this PR.

This PR changes the behavior of delete directive$patch: delete. Before: it deletes all entries that match the mergeKey. After: it deletes the first entry that matches the mergeKey.

The later approach doesn't solve all of the problems. Suppose user has a list like this:

- name: foo
  anotherField: abc
- name: foo
  anotherField: xyz

If the user wants to remove

- name: foo
  anotherField: xyz

there is no great way to express that. If using SMP, the user needs to delete the 1st entry, delete the 2nd entry and then add the 1st entry back.

I'm not sure if the new behavior is better than the existing behavior. @lavalamp @liggitt @apelisse WDYT?

Some background: @lavalamp suggested we should remove all matching entries: kubernetes/community#140 (comment) It is implemented in #38342

maybe we can delete last one, rather than the first one. Because when dulplicate envs exist, the first one is active.