feat(alm,install): adding in owner reference to deployment #84
Conversation
charltonaustin
force-pushed
the
ALM-142-add-owner-ref
branch
5 times, most recently
from
4791dcf
to
ed8aeb7
Compare
|
@charltonaustin it looks like the tests are having the same issue as the container |
install/deployment.go
Outdated
| Kind: ownerType.Kind, | ||
| Name: owner.GetName(), | ||
| UID: types.UID(uuid.NewUUID()), | ||
| Controller: &NewFalse, |
There was a problem hiding this comment.
I see this done two ways in k8s:
-
set descriptive variables right above and then take their address: https://sourcegraph.com/github.com/kubernetes/kubernetes@6d6b93986c9ac743ed18ac52dd4210bc55f1c737/-/blob/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/controller_ref.go#L44:2
-
a small helper function that returns a pointer (seems to be in tests only): https://sourcegraph.com/github.com/kubernetes/kubernetes@8e30314c95e8c42fbb4005ab38b70b488d3eddea/-/blob/plugin/pkg/admission/gc/gc_admission_test.go#L291:2
I would probably go with 1 for this?
There was a problem hiding this comment.
One thing about that for tests is then the object names don't match up, but I think I can rewrite the matcher to fix that.
install/deployment.go
Outdated
| BlockOwnerDeletion: &NewTrue, | ||
| }, | ||
| } | ||
| objectMeta := metav1.ObjectMeta{OwnerReferences: ownerReferences} |
There was a problem hiding this comment.
There's a SetOwnerReferences method on ObjectMeta
install/deployment.go
Outdated
| }, | ||
| } | ||
| objectMeta := metav1.ObjectMeta{OwnerReferences: ownerReferences} | ||
| dep := v1beta1.Deployment{Spec: spec, ObjectMeta: objectMeta} | ||
| dep.Namespace = owner.Namespace |
There was a problem hiding this comment.
(there's also a SetNamespace and SetGenerateName and SetLabels if you don't mind changing those while you're here)
There was a problem hiding this comment.
I don't mind at all.
install/deployment_test.go
Outdated
| if !reflect.DeepEqual(actual.Labels, expected.Labels) { | ||
| return false | ||
| } | ||
| if len(actual.OwnerReferences) != 1 { |
There was a problem hiding this comment.
Any reason not to reflect.DeepEqual ownerreferences?
There was a problem hiding this comment.
Yes because the UUID wasn't working, but if we are just using the parent obj uuid this problem goes away.
install/deployment.go
Outdated
| APIVersion: ownerType.APIVersion, | ||
| Kind: ownerType.Kind, | ||
| Name: owner.GetName(), | ||
| UID: types.UID(uuid.NewUUID()), |
There was a problem hiding this comment.
This id should be the uuid of the owning object, not a random one
install/deployment.go
Outdated
| const InstallStrategyNameDeployment = "deployment" | ||
| const ( | ||
| InstallStrategyNameDeployment = "deployment" | ||
|
|
charltonaustin
force-pushed
the
ALM-142-add-owner-ref
branch
3 times, most recently
from
62d6f27
to
994aead
Compare
- [ ] It works! - [ ] Comments provide sufficient explanations for the next contributor - [ ] Tests cover changes and corner cases - [ ] Follows Quay syntax patterns and format
charltonaustin
force-pushed
the
ALM-142-add-owner-ref
branch
from
994aead
to
2fb0aab
Compare
…owner-ref feat(alm,install): adding in owner reference to deployment
Description of Changes
Reviewer Checklist