-
Notifications
You must be signed in to change notification settings - Fork 55
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
manifest readiness update for odh-operator v2 #92
manifest readiness update for odh-operator v2 #92
Conversation
Skipping CI for Draft Pull Request. |
/retest |
931e95a
to
eabc672
Compare
Tested using instructions with Vedant's images in operator and in odh-model-controller:
|
/lgtm |
/retest |
/lgtm |
Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>
56e744c
to
fef83d8
Compare
Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>
fef83d8
to
89e5561
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.
I'm seeing two main differences, when comparing to odh-manifests
:
- Here, there is a new
kserve-prometheus-k8s
ClusterRole. Is it correct to be creating this resources? - In this PR, there is no
ServiceMonitor
. Is it OK to omit this resource? - $(monitoring-namespace) variable is not being replaced correctly.
This clusterrole was created for kserve, it was added in the kserve copy of odh-model-controller manifests, you can find it here
ServiceMonitor was already in
will take a look |
I was meaning that it is missing in the resulting manifests, not that is missing in the code. |
@israel-hdez I see the servicemonitor being generated when I run |
OK, so With this, I'm seeing only 2 issues:
|
Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>
Should be fixed now |
@VedantMahabaleshwarkar: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: heyselbi, Jooho, VedantMahabaleshwarkar 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 |
I will merge this manually because we need to update openshift-ci with this issue |
image is manually updated too.
|
Description
odh-manifests/odh-model-controller
and put them inconfig/
overlays/odh
andoverlays/dev
to handle the ODH overlays and also have an overlay option for local developmentkserve-enabled
flag, thus the same set of manifests can be used by both serving stacksTesting
Follow testing instructions on opendatahub-io/modelmesh-serving#237
Merge criteria: