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

Update Owners and Owners-aliases #869

Merged
merged 1 commit into from
Feb 26, 2024
Merged

Update Owners and Owners-aliases #869

merged 1 commit into from
Feb 26, 2024

Conversation

VaishnaviHire
Copy link
Member

Description

To expedite PR reviews and testing, added appropriate component owners to the repo.

How Has This Been Tested?

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@openshift-ci openshift-ci bot requested a review from etirelli February 15, 2024 23:37
@bartoszmajsak
Copy link
Contributor

@VaishnaviHire how would it work now? Would it automatically add all people from platform alias for every PR review? That would be a huge load on your side.

On a semi-related note - what is the process for reviewing PRs? Specifically where I'm looking for clarification is:

  • how many lgtms does a PR need (how many reviewers) to be considered mergeable?
  • who is responsible for merging the PR? clearly bot is not active (but maybe this will change it). should it be the author?
  • can I change automatically assigned reviewers to those e.g. suggested by GH (who most recently touched given code) or me wanting to share this knowledge with people I think are most affected or have the most expertise in the area

And a side quest to consider:

  • I am using conventional commits prefixes (or some subset of it), but there's no rule about it. Wondering if we want to leave it open or ask to have PR titles at least following this in this repository.

zdtsw
zdtsw reviewed Feb 16, 2024
View reviewed changes
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Feb 16, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zdtsw
Once this PR has been reviewed and has the lgtm label, please ask for approval from vaishnavihire. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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-ci openshift-ci bot added the lgtm label Feb 16, 2024
@VaishnaviHire
Copy link
Member Author

@VaishnaviHire how would it work now? Would it automatically add all people from platform alias for every PR review? That would be a huge load on your side.

On a semi-related note - what is the process for reviewing PRs? Specifically where I'm looking for clarification is:

  • how many lgtms does a PR need (how many reviewers) to be considered mergeable?
  • who is responsible for merging the PR? clearly bot is not active (but maybe this will change it). should it be the author?
  • can I change automatically assigned reviewers to those e.g. suggested by GH (who most recently touched given code) or me wanting to share this knowledge with people I think are most affected or have the most expertise in the area

Hi! I don't think it adds all the people. Following the guidelines here, the OWNERS_ALIASES file should now be parsed by tide. The goal was to atleast document list of reviewers for every component.

As follow up of this we will also fix the merge bot. I think 1 lgtm and 1 approval should be okay(let me know if you think otherwise). We need to remove the cherry-pick-approved label check to unblock automatic merges.

And a side quest to consider:

  • I am using conventional commits prefixes (or some subset of it), but there's no rule about it. Wondering if we want to leave it open or ask to have PR titles at least following this in this repository.

Maybe referencing this in Contributing guidelines will be helpful.

anishasthana
anishasthana reviewed Feb 21, 2024
View reviewed changes
OWNERS_ALIASES
- lucferbux
datasciencepipelines:
- HumairAK
modelregistry:
Copy link
Member

Choose a reason for hiding this comment

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

@dhirajsb should we have at least one more owner?

Copy link
Member Author

Choose a reason for hiding this comment

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

@dhirajsb Can you recommend anyone from model registry that we can add as reviewers?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tarilabs and @rareddy can be added as model-registry reviewers.

@openshift-ci openshift-ci bot removed the lgtm label Feb 21, 2024
@VaishnaviHire VaishnaviHire force-pushed the update_owners branch 2 times, most recently from 560c35b to 6e347a1 Compare February 21, 2024 19:52
anishasthana
anishasthana reviewed Feb 21, 2024
View reviewed changes
Copy link
Member

@anishasthana anishasthana left a comment

Choose a reason for hiding this comment

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

Nit: Quite a few of the rows are not sorted in alphabetical order.

harshad16
harshad16 reviewed Feb 24, 2024
View reviewed changes
Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

/lgtm
from my end.

@openshift-ci openshift-ci bot added the lgtm label Feb 24, 2024
@VaishnaviHire VaishnaviHire merged commit 7f477a3 into opendatahub-io:incubation Feb 26, 2024
6 of 7 checks passed
VaishnaviHire added a commit to VaishnaviHire/opendatahub-operator that referenced this pull request Feb 27, 2024
@VaishnaviHire
Update Owners and Owwners-aliases (opendatahub-io#869)
7a0e519 
(cherry picked from commit 7f477a3)
VaishnaviHire added a commit to VaishnaviHire/opendatahub-operator that referenced this pull request Feb 27, 2024
@VaishnaviHire
Update Owners and Owwners-aliases (opendatahub-io#869)
7210332 
(cherry picked from commit 7f477a3)
zdtsw referenced this pull request in red-hat-data-services/rhods-operator Mar 1, 2024
@VaishnaviHire @zdtsw
@iNecas @VedantMahabaleshwarkar
Sync kserve raw deployments (#202)
ddba9a8 
* Update Owners and Owwners-aliases (#869)

(cherry picked from commit 7f477a3)

* fix(DSC): do not reconcile resource if it has a special annotation (#879)

* fix(DSC+Kserve): do not reconcile inferenceservice-config for
rawdeployment

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* fix: wrong logic

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* feat(deploy): do not set ownerreference if resource has sepcial
annotation to false

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: change logic to --if kind is CRD do not care annotation

* update: change logic

- we still set ownerreference on resources
- we only skip reconcile if resource has defined annotation

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* fix(typo)

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* Update: limite scope of annoation to only kserve resource for now

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

---------

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
(cherry picked from commit 20f4ad0)

* fix(feature): use correct error variable name (#882)

(cherry picked from commit 0d8bd14)

* allow setting default deployment mode for Kserve in DSC (#864)

* allow setting default deployment mode for Kserve in DSC

Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>

* move kserve config logic to separate file + enhancements

Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>

* revert dev image set in operator CSV

Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>

* only setup kserve config if component is enabled

Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>

* bug fix

Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>

* address PR feedback

Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>

* cleanup

Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>

* fix lint error

Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>

* set default value for Kserve defaultDeploymentMode to be empty

Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>

* more pr feedback

Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>

* update bundle

Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>

* enhance documentation

Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>

* add readme for dev preview

Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>

---------

Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>
(cherry picked from commit cebd287)

* Update bundle

---------

Co-authored-by: Wen Zhou <wenzhou@redhat.com>
Co-authored-by: Ivan Necas <necasik@gmail.com>
Co-authored-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhirajsb dhirajsb dhirajsb left review comments

@ruivieira ruivieira ruivieira left review comments

@bartoszmajsak bartoszmajsak bartoszmajsak left review comments

@anishasthana anishasthana anishasthana left review comments

@HumairAK HumairAK HumairAK left review comments

@harshad16 harshad16 harshad16 left review comments

@zdtsw zdtsw zdtsw approved these changes

@etirelli etirelli Awaiting requested review from etirelli

Assignees

@zdtsw zdtsw

@harshad16 harshad16

Labels
lgtm
Projects
ODH Platform Planning
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@VaishnaviHire @bartoszmajsak @dhirajsb @ruivieira @zdtsw @anishasthana @HumairAK @harshad16