Skip to content

feat: Only sync controller owner references if an annotation is present#24814

Open
dprotaso wants to merge 1 commit intoargoproj:masterfrom
dprotaso:controller-refs
Open

feat: Only sync controller owner references if an annotation is present#24814
dprotaso wants to merge 1 commit intoargoproj:masterfrom
dprotaso:controller-refs

Conversation

@dprotaso
Copy link
Copy Markdown

@dprotaso dprotaso commented Oct 1, 2025 •
edited
Loading

This moves the rebase of argoproj/gitops-engine#771 into this repository

Part of #4764 #11972

@dprotaso dprotaso requested a review from a team as a code owner October 1, 2025 22:30
@bunnyshell
Copy link
Copy Markdown

bunnyshell bot commented Oct 1, 2025 •
edited
Loading

❌ Preview Environment undeployed from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@dprotaso
Only sync controller owner references if an annotation is present
1ca7d99 
Signed-off-by: Dave Protasowski <dprotaso@gmail.com>
This was referenced Oct 1, 2025
Open
Open
@dprotaso dprotaso changed the title Only sync controller owner references if an annotation is present [feat] Only sync controller owner references if an annotation is present Oct 1, 2025
@dprotaso dprotaso changed the title [feat] Only sync controller owner references if an annotation is present feat: Only sync controller owner references if an annotation is present Oct 1, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 1, 2025

Codecov Report

❌ Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@b8e8c1f). Learn more about missing BASE report.
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
gitops-engine/pkg/cache/references.go 0.00% 6 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff            @@
##             master   #24814   +/-   ##
=========================================
  Coverage          ?   60.79%           
=========================================
  Files             ?      404           
  Lines             ?    66224           
  Branches          ?        0           
=========================================
  Hits              ?    40262           
  Misses            ?    22719           
  Partials          ?     3243

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pjiang-dev
pjiang-dev suggested changes Oct 2, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@pjiang-dev pjiang-dev left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Could we add a unit test for the new sync option with resources with controller references on/off

Also this needs documentation for how to use the new sync option and why
Thanks

ppapapetrou76
ppapapetrou76 reviewed Oct 2, 2025
View reviewed changes
gitops-engine/pkg/cache/references.go
Comment on lines +28 to +37
if resource.HasAnnotationOption(un, common.AnnotationSyncOptions, common.SyncOptionControllerReferencesOnly) {
controllerOwnerRefs := []metav1.OwnerReference{}
for _, ownerRef := range un.GetOwnerReferences() {
if ownerRef.Controller != nil && *ownerRef.Controller {
controllerOwnerRefs = append(controllerOwnerRefs, ownerRef)
}
}
ownerRefs = controllerOwnerRefs
}

Copy link
Copy Markdown
Contributor

@ppapapetrou76 ppapapetrou76 Oct 2, 2025

Choose a reason for hiding this comment

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

Is there any UT we can add / modify to cover this extra if block?

Copy link
Copy Markdown
Author

@dprotaso dprotaso Oct 2, 2025

Choose a reason for hiding this comment

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

This was someone else's PR that I ported over now 2x. Can you reference an existing test I can look at?

@dprotaso
Copy link
Copy Markdown
Author

dprotaso commented Oct 2, 2025

Does it make sense to eventually have this option on by default and then the annotation lets people turn the feature off if they need it

@dprotaso
Copy link
Copy Markdown
Author

dprotaso commented Oct 2, 2025

/hold

See: #11972 (comment)

@ppapapetrou76 ppapapetrou76 mentioned this pull request Nov 3, 2025
Closed
6 tasks
@afarbos
Copy link
Copy Markdown
Contributor

afarbos commented Jan 21, 2026

Any update? this would be great!

@dprotaso
Copy link
Copy Markdown
Author

dprotaso commented Jan 21, 2026 •
edited
Loading

@afarbos can you confirm how to reproduce the issue?

That was my last question here: #11972 (comment)

Note - I'm looking to run the steps against a local kind cluster

@afarbos
Copy link
Copy Markdown
Contributor

afarbos commented Jan 24, 2026 •
edited
Loading

@afarbos can you confirm how to reproduce the issue?

That was my last question here: #11972 (comment)

Note - I'm looking to run the steps against a local kind cluster

I see, I have the issue with CAPI MachinePool all the time.

I would think trying the real think be hard. Couldn't you set the field manually?
I would create the resource from an example (https://cluster-api-aws.sigs.k8s.io/topics/machinepools.html#examples) and add the ownerReferences fields. CAPI just add apiversion, kind, name and uid. And it refer to the cluster like:

        "ownerReferences": [
            {
                "apiVersion": "cluster.k8s.io/v1alpha1",
                "kind": "Cluster",
                "name": "test1",
                "uid": "3d87808f-8bee-11e9-aff7-0242a75d7eaf"
            }
        ],

@dprotaso
Copy link
Copy Markdown
Author

dprotaso commented Jan 24, 2026 via email

@dprotaso dprotaso mentioned this pull request Mar 3, 2026
Open
14 tasks
@jalvarezit
Copy link
Copy Markdown

jalvarezit commented Mar 4, 2026

Is there any updates on this we would love to see this changes as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ppapapetrou76 ppapapetrou76 ppapapetrou76 left review comments

+1 more reviewer

@pjiang-dev pjiang-dev pjiang-dev requested changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dprotaso @afarbos @jalvarezit @ppapapetrou76 @pjiang-dev