-
Notifications
You must be signed in to change notification settings - Fork 54
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
Update to Operator SDK 1.1 #187
Conversation
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.
I just have some initial comments/questions, I'm going to test it on minikube/crc later today
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.
I was not able to test it on crc yet
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.
e2e tests need context.TODO() to be added in a few places. Check it with make test
name: manager | ||
namespace: system | ||
labels: | ||
control-plane: controller-manager |
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.
That's seems to be too general labels which may overlap with other generated controllers. What if we use here something devworkspace specific?
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.
I mostly left it because all kubebuilder projects are generated with this label and I didn't see anything about it in the documentation. We can probably remove 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.
I've removed the labels now and we're using only the ones from before. I still don't know what the purpose of the label is supposed to be, though.
I ran into some strange errors running on OpenShift today; I'm looking into the issue.
|
after adding some additional logs, I got
I've pushed changes to log such error 6ea3cff and going to provide fix for roles as well. P.S. After my fix:
|
Update dependencies and run go mod tidy
Re-bootstrap project to reflect kubebuilder project structure and port over existing files as required. Notable changes: * Directory structure is changed (deploy becomes config, controllers go in controllers/, etc.) * Kustomize is used to generate deploy files * Some auto-generated files are done differently (e.g. rbac is generated from annotations) Signed-off-by: Angel Misevski <[email protected]>
99cbe80
to
727afce
Compare
Rebased on master and squashed most fixups. Thanks @sleshchenko for figuring out the RBAC and testing code. I dropped the commit that added workspace roles; instead I've explicitly added the required roles to the main block of Tested on
|
862337e
to
f4466a3
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.
Tested cloudshell workspace on crc and it seems to work fine. So, I believe it should be good to proceed with it. Good work 👍
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.
Great work @amisevsk !
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amisevsk, davidfestal, sleshchenko 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 |
Signed-off-by: Angel Misevski <[email protected]>
Rework rbac annotations on controllers to reflect all requirements and regenerate rbac files. Signed-off-by: Angel Misevski <[email protected]>
Change naming for generated routes with openshift-oauth-proxy to avoid name collisions when multiple routes point to the same port (e.g. Theia webviews) Signed-off-by: Angel Misevski <[email protected]>
Signed-off-by: Angel Misevski <[email protected]>
f4466a3
to
fe87976
Compare
New changes are detected. LGTM label has been removed. |
Note this PR is a re-do of #186 based off a branch in this repo.
What does this PR do?
Update to Operator SDK 1.1.
This process requires bootstrapping a new project using kubebuilder and porting files over to the new structure. See this repo for a more detailed commit history (though it's still a bit messy and hard to read). I generally tried to avoid making unnecessary changes, to make the transition easier, so the configmap is still used, webhooks are done as they are currently, etc.
Main directory structure changes:
deploy/ => config/
config/default
config/devel/
config/crds/
config/components/
and combinedconfig/registry
*_controller.go
filespkg/apis/ => apis/
apis/
are named after the resource's group (controller
in our case)pkg/controller/ => controllers
controllers
are named for the group of the CR they control (controller
forComponents
,WorkspaceRoutings
;workspace
forDevWorkspaces
)controllers/controller
subdirectory is further divided to distinguish between the controller for Components and WorkspaceRoutings.pkg
now contains mostly helper code frompkg
in the current repoOther notable differences
devfile/api
, I reworked the script to grab the files. It now downloads to a temp dir and copies definitions to the expected directory (config/crds
). The copied files are gitignored.Further TODOs
devfile/api
, reflected by changes here. This is set ingo.mod
as a replace.e2e
tests.What issues does this PR fix or reference?
Resolves #180
We'll likely need to do additional fixes to finalize this work
Is it tested? How?
Tested on minikube; still need to test WTO and
crc
use cases