feat: add support for shared namespace ownership #1667
Conversation
Signed-off-by: Jayme Howard <[email protected]>
✅ Deploy Preview for docs-kargo-akuity-io ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@isugimpy we're pushing really hard to wrap up v0.5.0 right now, so I'm going to add this to the v0.6.0 milestone and we'll get to reviewing this in earnest probably next week. |
|
No worries! If I need it for testing I can always build locally. Good luck on 0.5.0! |
| ) | ||
| ns.OwnerReferences = []metav1.OwnerReference{*ownerRef} | ||
| ns.Labels[kargoapi.ProjectLabelKey] == kargoapi.LabelTrueValue { | ||
| if len(ns.OwnerReferences) == 0 { |
There was a problem hiding this comment.
Probably better to use a switch here:
switch {
case len(ns.OwnerReferences) == 0:
// Set owner reference
case ns.Labels[kargoapi.AllowSharedOwnershipLabelKey] == kargoapi.LabelTrueValue:
// Append owner reference
default:
// Return error
}
api/v1alpha1/labels.go
Outdated
| @@ -16,4 +16,6 @@ const ( | |||
| LabelTrueValue = "true" | |||
|
|
|||
| FinalizerName = "kargo.akuity.io/finalizer" | |||
|
|
|||
| AllowSharedOwnershipLabelKey = "kargo.akuity.io/allow-shared-ownership" | |||
There was a problem hiding this comment.
This is quite lengthy and cumbersome to type. Given this, I am wondering if it could for example be changed to kargo.akuity.io/shared-owner. Which gets the same message across.
Adjusted label value to be shorter Moved from if/else to switch/case Signed-off-by: Jayme Howard <[email protected]>
|
@hiddeco Thanks for the feedback! Addressed both as requested! |
Signed-off-by: Jayme Howard <[email protected]>
isugimpy
force-pushed
the
namespace_adoption
branch
from
49af585
to
85e6123
Compare
|
One quick clarifying thing. I neglected to set the controller property on the ownerReference to false before. This fixes that to make sure there's no ownership collision there. If that's not desirable, would love to discuss further. |
|
Test driving. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1667 +/- ##
==========================================
+ Coverage 43.81% 44.18% +0.37%
==========================================
Files 202 202
Lines 12935 13061 +126
==========================================
+ Hits 5667 5771 +104
- Misses 7026 7045 +19
- Partials 242 245 +3 ☔ View full report in Codecov by Sentry. |
|
@isugimpy have you gotten this to work end-to-end? Similar changes should be required to the Project webhook... It handles synchronous Project initialization, which is often required if someone apiVersion: kargo.akuity.io/v1alpha1
kind: Project
metadata:
name: kargo-demo
---
# Many more resources that need to go in the kargo-demo namespace, which must exist |
|
@isugimpy I think this just needs some webhook changes. If you're busy, I'm happy to take this the last mile. Your call. |
|
As we're approaching a v0.6.0 release and this non-critical PR has outstanding work still, deferring this to v0.7.0. |
|
@krancour Hey, I'm sorry about not responding. Been on vacation and didn't see this. I don't think I did a full end to end of this, actually. I remember testing briefly, but it's been a month or so and it's a bit fuzzy. You're quite welcome to take this across the line if you'd like! I probably won't be able to get back to it for a little bit. |
|
@isugimpy you did all the hard work. Will try to get this over the line in the v0.7.0 timeframe. |
|
Closing in favor of #2076 |
Fixes #1654
Open to changing this in whatever way makes sense. Thanks for the quick feedback on the issue!