Skip to content
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

Upgrade operator-sdk to v1.x #136

Merged
merged 40 commits into from
Aug 18, 2021
Merged

Conversation

chetan-rns
Copy link
Member

@chetan-rns chetan-rns commented Jun 21, 2021

What type of PR is this?

/kind enhancement

What does this PR do / why we need it:

This PR upgrades the operator-sdk version to v1.x thereby supporting the new kubebuilder layout. Changes are made in accordance to the official migration guide.

Changes:

  1. Controllers are migrated from pkg/controller/<kind> to /controllers
  2. APIs are migrated from pkg/apis/<group>/<version> to /api/<version>
  3. Manifests are migrated from deploy to config
  4. RBAC is configured as markers

Have you updated the necessary documentation?

  • Documentation update is required by this PR.
  • Documentation has been updated.

Which issue(s) this PR fixes:

Fixes #?

Test acceptance criteria:

  • Unit Test
  • E2E Test

How to test changes / Special notes to the reviewer:

  • Run the operator locally
1. oc new-project gitops-operator-system
2. make install run
  • Deploy using OLM
1 make docker-build docker-push IMG="example.com/memcached-operator:v0.0.1"
2. make bundle IMG="example.com/memcached-operator:v0.0.1"
3. make bundle-build bundle-push
4. operator-sdk run bundle example.com/memcached-operator-bundle:v0.0.1
5. Cleanup: operator-sdk cleanup memcached-operator
  • Run e2e tests
make test-e2e

Ref: https://sdk.operatorframework.io/docs/building-operators/golang/quickstart/

@openshift-ci
Copy link

openshift-ci bot commented Jun 21, 2021

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@chetan-rns
Copy link
Member Author

@wtam2018 @iam-veeramalla @shubhamagarwal19 The sdk has created a service account with the name controller-manager which is referred eveyrwhere. Do we use this new naming convention or switch back to the name gitops-operator?

@iam-veeramalla
Copy link
Collaborator

@wtam2018 @iam-veeramalla @shubhamagarwal19 The sdk has created a service account with the name controller-manager which is referred eveyrwhere. Do we use this new naming convention or switch back to the name gitops-operator?

I think we need to continue with gitops-operator. Changing the service account name will be a breaking change which I dont think is a good thing to consider for now.

@shubhamagarwal19
Copy link
Contributor

@wtam2018 @iam-veeramalla @shubhamagarwal19 The sdk has created a service account with the name controller-manager which is referred eveyrwhere. Do we use this new naming convention or switch back to the name gitops-operator?

I think we need to continue with gitops-operator. Changing the service account name will be a breaking change which I dont think is a good thing to consider for now.

Completely agree with abhishek's point, i don't think that we should update the service account name either !!

@chetan-rns
Copy link
Member Author

/test v4.7-e2e

@chetan-rns
Copy link
Member Author

/test v4.8-e2e

@wtam2018
Copy link
Collaborator

It looks good in general. I wonder if we can gnerate the base manifests (config/crc/bases) then use kustomize to generate the manifests we want (like the make bundletarget does). It looks like you have put the desire cvs in the config/crd/bases?

Makefile Show resolved Hide resolved
go.mod Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
scripts/run_e2e_tests.sh Outdated Show resolved Hide resolved
scripts/run_e2e_tests.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@amitkrout amitkrout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chetan-rns Thanks for addressing the review comments. I have tested the make targets and test scripts.

/approve

@wtam2018 @iam-veeramalla @shubhamagarwal19 Can you please review the pr and apply the /lgtm label if looks good

Copy link
Collaborator

@wtam2018 wtam2018 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm label Aug 18, 2021
@wtam2018 wtam2018 merged commit 25788e7 into redhat-developer:master Aug 18, 2021
@openshift-ci
Copy link

openshift-ci bot commented Aug 18, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amitkrout, wtam2018

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

trdoyle81 pushed a commit to trdoyle81/gitops-operator that referenced this pull request Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants