Skip to content

v3 backport - fix --take-ownership#30768

Merged
scottrigby merged 2 commits intohelm:dev-v3from
banjoh:em/fix-take-ownership
Apr 21, 2025
Merged

v3 backport - fix --take-ownership#30768
scottrigby merged 2 commits intohelm:dev-v3from
banjoh:em/fix-take-ownership

Conversation

@banjoh
Copy link
Copy Markdown
Contributor

@banjoh banjoh commented Apr 17, 2025

v3 back port for #30697

If a resource exists in the cluster and is to be adopted by helm install
--take-ownership, it is left unchanged while helm reports the
installation to have succeeded.

This is due to CRs and CRDs being merged without three-way-merge, which
results in an empty patch.

By using a three-way-merge transparently when --take-ownership is used,
the helm behaves as expected without breaking previous behavior.

Fixes helm#30622

Signed-off-by: Patrick Seidensal <pseidensal@suse.com>
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 17, 2025
Signed-off-by: Evans Mungai <mbuevans@gmail.com>
@banjoh banjoh force-pushed the em/fix-take-ownership branch from 018c294 to 0906fe7 Compare April 17, 2025 17:19
Copy link
Copy Markdown
Member

@scottrigby scottrigby left a comment

Choose a reason for hiding this comment

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

diff looks good (between v4 and v3)

diff -y <(gh pr diff 30697) <(gh pr diff 30768)

Manual test with steps from #30622 also works as well. Exact commands if anyone wants to follow along:

  1. create CRD

    kubectl apply -f - <<EOF
    apiVersion: apiextensions.k8s.io/v1
    kind: CustomResourceDefinition
    metadata:
      name: datas.crd.com
    spec:
      group: crd.com
      scope: Namespaced
      names:
        plural: datas
        singular: data
        kind: Data
      versions:
      - name: v1
        served: true
        storage: true
        schema:
          openAPIV3Schema:
            type: object
            x-kubernetes-preserve-unknown-fields: true
    EOF
  2. apply a CR (removed ns from original example)

    kubectl apply -f - <<EOF
    kind: Data
    apiVersion: crd.com/v1
    metadata:
      name: crd-test
      labels:
        test: adopt
    spec:
      color: blue
      size: large
    EOF
  3. build helm binary from this PR

    binary will be in ./bin/helm

    gh pr checkout 30768
    make clean build
  4. create and install chart containing a different version of that same CR

    name=pr-30768
    mkdir -p $name/templates
    
    cat > $name/Chart.yaml <<EOF
    apiVersion: v2
    name: $name
    version: 0.1.0
    EOF
    
    cat > $name/templates/test.yaml <<EOF
    kind: Data
    apiVersion: crd.com/v1
    metadata:
      name: crd-test
      labels:
        test: adopt
    spec:
      weight: heavy
    EOF
    
    ./bin/helm install pr-30768 pr-30768/ --take-ownership
  5. ensure generation has incremented and spec values have been merged

    kubectl get datas.crd.com crd-test -oyaml

    you should see

    metadata:
      generation: 2

    and

    spec:
      color: blue
      size: large
      weight: heavy

@scottrigby scottrigby added bug Categorizes issue or PR as related to a bug. Has One Approval This PR has one approval. It still needs a second approval to be merged. labels Apr 17, 2025
@scottrigby scottrigby changed the title Backport - fix --take-ownership v3 backport - fix --take-ownership Apr 18, 2025
Copy link
Copy Markdown
Contributor

@sabre1041 sabre1041 left a comment

Choose a reason for hiding this comment

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

LGTM. Confirmed same results with the instructions/steps provided by @scottrigby

@scottrigby scottrigby merged commit e4bc4a3 into helm:dev-v3 Apr 21, 2025
5 checks passed
@scottrigby scottrigby removed the Has One Approval This PR has one approval. It still needs a second approval to be merged. label Apr 21, 2025
@scottrigby scottrigby added this to the 3.18.0 milestone Apr 21, 2025
@banjoh banjoh deleted the em/fix-take-ownership branch August 1, 2025 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants