-
Notifications
You must be signed in to change notification settings - Fork 49
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
Internal hostname gateway follow up #229
Internal hostname gateway follow up #229
Conversation
Signed-off-by: jooho lee <jlee@redhat.com>
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.
Just one small fix, and then I think this is ready to go!
go.sum
Outdated
@@ -6,86 +6,268 @@ cloud.google.com/go v0.44.2/go.mod h1:60680Gw3Yr4ikxnPRS/oxxkBccT6SA1yMk63TGekxK | |||
cloud.google.com/go v0.44.3/go.mod h1:60680Gw3Yr4ikxnPRS/oxxkBccT6SA1yMk63TGekxKY= | |||
cloud.google.com/go v0.110.10 h1:LXy9GEO+timppncPIAZoOj3l58LIU9k+kn48AN7IO3Y= | |||
cloud.google.com/go v0.110.10/go.mod h1:v1OoFqYxiBkUrruItNM3eT4lLByNjxmJSV/xDKJNnic= | |||
cloud.google.com/go/accessapproval v1.7.4/go.mod h1:/aTEh45LzplQgFYdQdwPMR9YdX0UlhBmvB84uAmQKUc= |
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 not sure how all these changes land here... Since it is a lot, please, confirm if you want them.
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.
interesting, why keep changing this ? I think we should add git pre commit or Make record to do go mod tidy
by default. anyway I updated for now
@@ -237,7 +238,10 @@ func (r *KserveGatewayReconciler) copyServingCertSecretFromIsvcNamespace(ctx con | |||
Name: fmt.Sprintf("%s-%s", sourceSecret.Name, sourceSecret.Namespace), | |||
Namespace: meshNamespace, | |||
Labels: map[string]string{ | |||
"opendatahub.io/managed": "true", |
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.
In my original comment, my suggestion was to add the recommended labels, rather than replace this one 🙂
I almost can bet you will need "opendatahub.io/managed": "true"
for the clean-up. I'll try it.
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.
Right, it needs the label. Added
Signed-off-by: jooho lee <jlee@redhat.com>
0b9e9fe
to
e818f62
Compare
/retest |
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: israel-hdez, Jooho 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 |
Description
The original PR was accidentally merged so I create another pr to follow up the ticket.(#221)
How Has This Been Tested?
Merge criteria: