-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ownerRef into kustomize build #94
Add ownerRef into kustomize build #94
Conversation
/lgtm |
very well structured change, thank you @rajansandeep ! /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johnsonj, rajansandeep The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@@ -136,33 +136,12 @@ func (r *Reconciler) reconcileExists(ctx context.Context, name types.NamespacedN | |||
} | |||
var manifestStr string | |||
|
|||
if r.IsKustomizeOptionUsed() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI @johnsonj and @rajansandeep,
I'd like to share that this change broke the projects built with Kubebuilder which are using the addons. I am able to check it by running the script to gen the testdata which will scaffold a project that gets the latest version of this project.
Now, when we try to run make generate with here it gets stuck. So, far shows that controller-gen is not working with.
NOTE The same was not faced with the version sigs.k8s.io/kubebuilder-declarative-pattern v0.0.0-20200512162422-ce639cbf6d4c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE The same was not faced with the version sigs.k8s.io/kubebuilder-declarative-pattern v0.0.0-20200512162422-ce639cbf6d4c
IIUC, the mentioned version includes changes from this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find! @camilamacedo86 can you confirm that it does succeed with ce639cbf6d4ce402273ad35ecd5f2996758b4f6b
(sigs.k8s.io/kubebuilder-declarative-pattern v0.0.0-20200512162422-ce639cbf6d4c
)
We've only had docs changes since that commit, so I wouldn't expect anything since then to have broken it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @johnsonj,
Yes, I can confirm that it works with sigs.k8s.io/kubebuilder-declarative-pattern v0.0.0-20200512162422-ce639cbf6d4c
.
We've only had docs changes since that commit, so I wouldn't expect anything since then to have broken it.
Exactly, because of this, I ping here. I did not saw anything after that which could cause the issue.
So, just to clarifies. The Kubebuilder did not have any change in the period and I just not that I was unable to gen the mock testdata dir with its script. There, IMO the fix would be kept the version fixed. However, it might affect others as well. because of this, I decided to ping. I am trying to figure out as well.
I hope that I could clarify. Please, feel free to reach me out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @johnsonj and @rajansandeep,
The issue was with go mod cache.
After cleaning it as follows worked .. sorted out .. sorry for the false alert :-)
$ export GO111MODULE=off
$ go clean --modcache
$ go clean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camilamacedo86 Thank you for confirming :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome! best kind of fix :). Though this brings up: if we can add any CI to this repo to ensure we don't break kubebuilder I am interested!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trolled by the GO cache.
IMO the solution could be Kubebuilder used a fixed version: https://github.com/kubernetes-sigs/kubebuilder/pull/1527/files. However, a CI here which uses kb to check the changes also shows a great idea indeed to verify this project. 👍
The manifest built by Kustomize doesn't include the OwnerRefs. Due to this, the manifest built by Kustomize doesn't reconcile with the operator.
This PR ensures reconciliation by parsing the manifest created by Kustomize into objects and then injecting the OwnerRef needed for Reconcile.
Related to #56