-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Added CONTAINER_TOOL to set container tool for operator build (#6089) #6090
Added CONTAINER_TOOL to set container tool for operator build (#6089) #6090
Conversation
387e812
to
be67fd8
Compare
@stanislavulrych if you don't mind adding a changelog for this PR. The contents of the PR looks great. |
@jmrodri Added. Please see the last commit. |
ce8b4bd
to
71f2a2f
Compare
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 @stanislavulrych, thanks for the contribution! I really like this change since it gives an operator author more flexibility in the tooling they choose to use :)
I noticed a couple things:
- A minor typo in the changelog
- There are changes to the scaffolds, so that means we need to regenerate our testdata using
make generate
. This will help us keep the testdata up to date.
…or-framework#6089) Signed-off-by: Stanislav Ulrych <stanislav.ulrych@getmanta.com>
…or-framework#6089) Signed-off-by: Stanislav Ulrych <stanislav.ulrych@getmanta.com>
Co-authored-by: Bryce Palmer <everettraven@gmail.com> Signed-off-by: Stanislav Ulrych <stanislav.ulrych@getmanta.com>
9bd1a63
to
e6fc062
Compare
…or-framework#6089) Signed-off-by: Stanislav Ulrych <stanislav.ulrych@gmail.com>
…or-framework#6089) Signed-off-by: Stanislav Ulrych <stanislav.ulrych@gmail.com>
@everettraven originally, I intended to only implement this change for go based operators. But from the running of However, I can still see there are docker targets hardcoded and the resulting test code is not as I expect it. Specifically, there are targets using |
Hi @stanislavulrych I think it is fine for the changes to go into the ansible and helm operators as well, but now that I think about it the Go operator scaffolding is done via Kubebuilder. Operator SDK pulls in Kubebuilder's CLI in and extends it to add the ansible and helm plugins but the go/v3 plugin is maintained as part of the Kubebuilder source code. Any changes to the scaffolds for a Go based operator would have to be done in https://github.com/kubernetes-sigs/kubebuilder (aside from the If you are still interested in continuing to contribute this functionality, we can hold this PR on the necessary changes being made in Kubebuilder and then updating this PR to contain the necessary changes for SDK (I presume that most of these changes will stay the same, but I would personally rather wait until the changes are merged into Kubebuilder and pulled into SDK before we commit these changes just in case something needs to be changed slightly).
This would also have to be a change made in Kubebuilder. There would likely need to be a bit more discussion as to how to handle this properly over there, but as a reviewer/maintainer in Kubebuilder I would be happy to help discuss any changes that need to be made. |
Hi @everettraven I agree and I will try to open a respective pull request in kubebuilder then. |
@stanislavulrych Sounds good. I will go ahead and add a hold to this PR for the time being. Thanks for the contributions! /hold |
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
@openshift-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Implementation of #6089