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.

Sign up for GitHub

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

Jump to bottom

feat: moves kubebuilder directives to its own file #485

Merged
merged 3 commits into from
Aug 30, 2023
Merged

feat: moves kubebuilder directives to its own file #485

merged 3 commits into from
Aug 30, 2023

Conversation

bartoszmajsak
Copy link
Contributor

@bartoszmajsak bartoszmajsak commented Aug 28, 2023 •
edited
Loading

This way it is easier to go through the controller/reconcile logic, as we do not have to scroll almost 200 lines down :)

Contains fixes from #484 and #483, but not the updated role.yaml

@openshift-ci openshift-ci bot requested review from LaVLaS and VaishnaviHire August 28, 2023 07:13
@bartoszmajsak
feat: moves kubebuilder directives to its own file
75bd8ec 
This way it is easier to go through the controller/reconcile logic, as
e do not have to scroll almost 200 lines down :)
@bartoszmajsak
Copy link
Contributor Author

@VaishnaviHire @zdtsw any thoughts on this change?

zdtsw
zdtsw approved these changes Aug 29, 2023
View reviewed changes
Copy link
Member

@zdtsw zdtsw left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Aug 29, 2023
@zdtsw
Copy link
Member

zdtsw commented Aug 29, 2023

@bartoszmajsak how you feel to get #492 into your PR?

@zdtsw @bartoszmajsak
Update RBAC permission for resource
dd861ca 
- add more comments
- remove securityistio.io
- remove telemetry,istio.io
- remove downstream only required permission

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm label Aug 29, 2023
@bartoszmajsak
Copy link
Contributor Author

@zdtsw I cherry-picked your commit and did one fix (typo patchdelete with missing ;). Perhaps you can close your PR and we squash merge this one?

@zdtsw
Copy link
Member

zdtsw commented Aug 29, 2023

@zdtsw I cherry-picked your commit and did one fix (typo patchdelete with missing ;). Perhaps you can close your PR and we squash merge this one?

cool! thanks for handling typo mistake

@zdtsw zdtsw mentioned this pull request Aug 29, 2023
Closed
3 tasks
zdtsw
zdtsw approved these changes Aug 29, 2023
View reviewed changes
Copy link
Member

@zdtsw zdtsw left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Aug 29, 2023
@VaishnaviHire
Copy link
Member

@bartoszmajsak This PR needs a rebase

@bartoszmajsak
Copy link
Contributor Author

@bartoszmajsak This PR needs a rebase

I will resolve conflicts in all my opened PRs tomorrow morning.

@openshift-ci openshift-ci bot removed the lgtm label Aug 30, 2023
@bartoszmajsak
Copy link
Contributor Author

@VaishnaviHire this is now resolved and can be squashed to main

@zdtsw
Copy link
Member

zdtsw commented Aug 30, 2023

@VaishnaviHire this is now resolved and can be squashed to main

thanks @bartoszmajsak
i am gonna do a final test based on this PR if all good i will approve it again.

@zdtsw
Copy link
Member

zdtsw commented Aug 30, 2023

test with quay.io/wenzhou/opendatahub-operator-catalog:v2.8.485 looks good

zdtsw
zdtsw approved these changes Aug 30, 2023
View reviewed changes
Copy link
Member

@zdtsw zdtsw left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Aug 30, 2023
etirelli
etirelli approved these changes Aug 30, 2023
View reviewed changes
Copy link
Contributor

@etirelli etirelli left a comment

Choose a reason for hiding this comment

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

lgtm

etirelli
etirelli approved these changes Aug 30, 2023
View reviewed changes
Copy link
Contributor

@etirelli etirelli left a comment

Choose a reason for hiding this comment

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

lgtm

@openshift-ci
Copy link

openshift-ci bot commented Aug 30, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: etirelli, zdtsw

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

@openshift-merge-robot openshift-merge-robot merged commit 2fa7f69 into opendatahub-io:main Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zdtsw zdtsw zdtsw approved these changes

@etirelli etirelli etirelli approved these changes

@LaVLaS LaVLaS Awaiting requested review from LaVLaS

@VaishnaviHire VaishnaviHire Awaiting requested review from VaishnaviHire

Assignees

@etirelli etirelli

@zdtsw zdtsw

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bartoszmajsak @zdtsw @VaishnaviHire @etirelli @openshift-merge-robot