search

karmada-io / karmada

Open, Multi-Cloud, Multi-Cluster Kubernetes Orchestration
https://karmada.io
Apache License 2.0
4.12k stars 806 forks source link

Fix promoting failed if a resource with other kind used same name has promoted before #1824

Closed wuyingjun-lucky closed 1 year ago

wuyingjun-lucky commented 1 year ago

Signed-off-by: wuyingjun wuyingjun_yewu@cmss.chinamobile.com

What type of PR is this? /kind bug

What this PR does / why we need it: Fix promoting failed if a resource with other kind used same name has promoted before Which issue(s) this PR fixes: Fixes https://github.com/karmada-io/karmada/issues/1821

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmadactl`: fixed promoting failed issue if a resource with another kind using the same name has been promoted before.
RainbowMango commented 1 year ago

/assign @lonelyCZ And also please confirm if this patch should be picked to release branches.

lonelyCZ commented 1 year ago

Hi, @wuyingjun-lucky , please change your pr description

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

to

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

It will close the #1821 automatically, when this pr is merged.

wuyingjun-lucky commented 1 year ago

Hi, @wuyingjun-lucky , please change your pr description

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

to

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

It will close the #1821 automatically, when this pr is merged.

OK， GOT it

lonelyCZ commented 1 year ago

Whether is there a situation that the CR has same Kind, but different Group or Version? Should we consider this situation?

@RainbowMango @wuyingjun-lucky

wuyingjun-lucky commented 1 year ago

Whether is there a situation that the CR has same Kind, but different Group or Version? Should we consider this situation?

@RainbowMango @wuyingjun-lucky I think it may happen

wuyingjun-lucky commented 1 year ago

Whether is there a situation that the CR has same Kind, but different Group or Version? Should we consider this situation?

@RainbowMango @wuyingjun-lucky

use kind-group-version-name-pp ? feel tedious. @lonelyCZ

lonelyCZ commented 1 year ago

use kind-group-version-name-pp ?

I also think it is tedious. Using hash code?

How do you think? @RainbowMango

RainbowMango commented 1 year ago

Post my comment at https://github.com/karmada-io/karmada/issues/1821#issuecomment-1130958402.

wuyingjun-lucky commented 1 year ago

/cc @lonelyCZ

lonelyCZ commented 1 year ago

Thanks, @wuyingjun-lucky

I will review it ASAP.

wuyingjun-lucky commented 1 year ago

/cc @lonelyCZ

wuyingjun-lucky commented 1 year ago

/assgin @lonelyCZ

lonelyCZ commented 1 year ago

Thanks @wuyingjun-lucky

I will review it ASAP in these days.

wuyingjun-lucky commented 1 year ago

Thanks @wuyingjun-lucky

I will review it ASAP in these days.

Sorry, I found I lost some reviews , I will resolve it and then assign it to you

lonelyCZ commented 1 year ago

I just tested it that worked fine.

/lgtm

wuyingjun-lucky commented 1 year ago

/assign @RainbowMango

RainbowMango commented 1 year ago

/approve

/hold for:

  1. need update release notes in the PR description
  2. should we pick this patch to release branches? @lonelyCZ
karmada-bot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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: - ~~[OWNERS](https://github.com/karmada-io/karmada/blob/master/OWNERS)~~ [RainbowMango] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
lonelyCZ commented 1 year ago

should we pick this patch to release branches?

We can pick it, but there is not karmadactl e2e test before v1.2, should we pick it for v1.0 and v1.1?

RainbowMango commented 1 year ago

We can pick it, but there is not karmadactl e2e test before v1.2, should we pick it for v1.0 and v1.1?

It will be great if we can do that.

wuyingjun-lucky commented 1 year ago

We can pick it, but there is not karmadactl e2e test before v1.2, should we pick it for v1.0 and v1.1?

It will be great if we can do that.

Does cherry_pick.sh support cherry pick a commit with diff file not exist in release branch? Or do you mean I can cherry pick the commit manually ? And check the release note karmadactl: fixed karmadactl promoting failed if a resource with other kind used same name has promoted before @RainbowMango

wuyingjun-lucky commented 1 year ago

We can pick it, but there is not karmadactl e2e test before v1.2, should we pick it for v1.0 and v1.1?

It will be great if we can do that.

Does cherry_pick.sh support cherry pick a commit with diff file not exist in release branch? Or do you mean I can cherry pick the commit manually ? And check the release note karmadactl: fixed karmadactl promoting failed if a resource with other kind used same name has promoted before @RainbowMango

help to check @RainbowMango

RainbowMango commented 1 year ago

Does cherry_pick.sh support cherry pick a commit with diff file not exist in release branch?

Yes, I think so. actually, the script doesn't care if the file exist or not, it just care if there is a conflict, If there is a conflict it will stop and wait for the conflict to be solved and then continue.

do you mean I can cherry pick the commit manually ?

Yes, you can, but the cherry_pick.sh runs extremely similar to human beings, I recommend you try it first.

RainbowMango commented 1 year ago

/hold cancel

I guess we are ready to go now for don't block the cherry-pick work.