Skip ownership check for resources with exists annotation
#522
Conversation
praveenrewar
force-pushed
the
exists-annotation
branch
from
8bfcb58 to
ac89b39
Compare
praveenrewar
force-pushed
the
exists-annotation
branch
from
ac89b39 to
04cd944
Compare
| @@ -144,7 +149,9 @@ func (a *LabeledResources) checkResourceOwnership(resources []Resource, opts All | |||
| var errs []error | |||
|
|
|||
| for _, res := range resources { | |||
| if val, found := res.Labels()[expectedLabelKey]; found { | |||
| val, hasExpectedLabelKey := res.Labels()[expectedLabelKey] | |||
| _, hasExistsAnnotation := res.Annotations()[ExistsAnnKey] | |||
There was a problem hiding this comment.
I just realised that the res over here is an existing resource and not a new resource. The test case is passing because the exists ann is also added to the resource deployed via kubectl. I will make relevant changes.
praveenrewar
force-pushed
the
exists-annotation
branch
from
04cd944 to
c0a054e
Compare
Skip ownership check for resources with kapp.k14s.io/exists annotation and kapp.k14s.io/noop annotation
praveenrewar
force-pushed
the
exists-annotation
branch
from
c0a054e to
3d2548d
Compare
| @@ -135,6 +143,28 @@ func (a *LabeledResources) AllAndMatching(newResources []Resource, opts AllAndMa | |||
| return resources, nil | |||
| } | |||
|
|
|||
| func (a *LabeledResources) resourcesForOwnershipCheck(newResources []Resource, nonLabeledResources []Resource) []Resource { | |||
There was a problem hiding this comment.
nit: can drop receiver name a
| for _, res := range nonLabeledResources { | ||
| if !resourcesToBeSkipped[NewUniqueResourceKey(res).String()] { | ||
| resources = append(resources, res) | ||
| } | ||
| } |
There was a problem hiding this comment.
dont we actually want to verify the ownership of non-new resources as well? or is this not how it works today since because we only find non labeled resources based on new resources?
There was a problem hiding this comment.
(I have read it a few times, but I am still not sure if I have understood it properly 😅 )
or is this not how it works today since because we only find non labeled resources based on new resources?
Yeah, we find the non labeled resources based on the new resources and the labeled resources.
https://github.com/vmware-tanzu/carvel-kapp/blob/31e7abfc48f00ab81fda9ca0235062e9e33ca7c1/pkg/kapp/resources/labeled_resources.go#L213
What this PR does / why we need it:
Skip ownership check for resources with
kapp.k14s.io/existsandkapp.k14s.io/noopannotations as these resources could be created by another kapp app.Which issue(s) this PR fixes:
Fixes #457
Does this PR introduce a user-facing change?
Additional Notes for your reviewer:
Review Checklist:
a link to that PR
change
Additional documentation e.g., Proposal, usage docs, etc.: