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

chore: renames IngressGatewaySpec type to GatewaySpec #1144

Conversation

bartoszmajsak
Copy link
Contributor

@bartoszmajsak bartoszmajsak commented Jul 29, 2024

Description

The original name is confusing as it does not define anything related to the ingress. It configures Istio Gateway resource which is attached to ingress service in case of Model Serving.

Important

This change makes the change of the Golang type only. It does not change publicly exposed ingressGateway config defined for KServe component under DataScienceCluster.

How Has This Been Tested?

Screenshot or short clip

Merge criteria

  • You have read the contributors guide.
  • Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
  • Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
  • 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 review from ajaypratap003 and grdryn July 29, 2024 15:59
@@ -246,7 +246,7 @@ spec:
certificate:
description: Certificate specifies configuration of
the TLS certificate securing communications of the
for Ingress Gateway.
for gateway.
Copy link
Member

Choose a reason for hiding this comment

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

should not be a change in bundle/manifests ?

Copy link
Contributor Author

@bartoszmajsak bartoszmajsak Jul 29, 2024

Choose a reason for hiding this comment

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

The godoc changed because the type itself is not really about "ingress". It is Istio Gateway resource which can applied on ingress or egress service. Let me review it more thoroughly though. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that Check config and readme updates / Ensure generated files are included (pull_request) job is not commenting on the PR (anymore?)

https://github.com/opendatahub-io/opendatahub-operator/actions/runs/10147710041/job/28058754754#step:5:12

/cc @AjayJagan

@bartoszmajsak bartoszmajsak force-pushed the rename-ingress-gateway-type branch from 3d3e6c9 to 7cfbae0 Compare July 29, 2024 16:24
@openshift-ci openshift-ci bot requested a review from AjayJagan July 29, 2024 16:39
@bartoszmajsak bartoszmajsak removed the request for review from AjayJagan July 29, 2024 16:40
Original name is confusing as it does not actually define anything
related to the ingress. It is configuring Istio Gateway resource which
is, in case of Model Serving, attached to ingress service.

This change makes the change of the golang type, but not publicly
exposed `ingressGateway` config defined for KServe component.
@bartoszmajsak bartoszmajsak force-pushed the rename-ingress-gateway-type branch from 7cfbae0 to f943ac5 Compare July 30, 2024 06:52
@bartoszmajsak
Copy link
Contributor Author

Putting on hold to prevent auto-merge. I would like @israel-hdez to weigh in

Copy link
Contributor

@israel-hdez israel-hdez left a comment

Choose a reason for hiding this comment

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

This is fine to me. Certainly, the type is applicable to either Ingress or Egress gateways.
In the exposed API, though, I think the ingress on the name do is relevant.

Copy link

openshift-ci bot commented Aug 1, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: israel-hdez, 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-bot openshift-merge-bot bot merged commit e80dcbe into opendatahub-io:incubation Aug 1, 2024
8 checks passed
zdtsw referenced this pull request in zdtsw-forking/rhods-operator Aug 2, 2024
…ervices#1144)

Original name is confusing as it does not actually define anything
related to the ingress. It is configuring Istio Gateway resource which
is, in case of Model Serving, attached to ingress service.

This change makes the change of the golang type, but not publicly
exposed `ingressGateway` config defined for KServe component.
zdtsw referenced this pull request in zdtsw-forking/rhods-operator Aug 2, 2024
…ervices#1144)

Original name is confusing as it does not actually define anything
related to the ingress. It is configuring Istio Gateway resource which
is, in case of Model Serving, attached to ingress service.

This change makes the change of the golang type, but not publicly
exposed `ingressGateway` config defined for KServe component.

(cherry picked from commit e80dcbe)
openshift-merge-bot bot referenced this pull request in red-hat-data-services/rhods-operator Aug 2, 2024
Original name is confusing as it does not actually define anything
related to the ingress. It is configuring Istio Gateway resource which
is, in case of Model Serving, attached to ingress service.

This change makes the change of the golang type, but not publicly
exposed `ingressGateway` config defined for KServe component.

(cherry picked from commit e80dcbe)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants