-
Notifications
You must be signed in to change notification settings - Fork 76
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
Add support to override service endpoints #1648
Conversation
✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
/cc @Prajyot-Parab |
pkg/endpoints/endpoints.go
Outdated
// TransitGateway service. | ||
TransitGateway serviceID = "TransitGateway" | ||
// COS service. | ||
COS serviceID = "COS" |
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.
Is it required for this to be camelcase or we can have it in lowercase as other IDs to maintain uniformity.
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.
Changed to be consistent with existing endpoints.
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.
@Prajyot-Parab how is aws or other vendors are handling this?
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.
@mkumatag everything in smallcase
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.
for a reference, can you please record that 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.
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.
- Update the docs
- Add the UT
@@ -144,6 +150,16 @@ func FetchRCEndpoint(serviceEndpoint []ServiceEndpoint) string { | |||
return "" | |||
} | |||
|
|||
// FetchEndpoints returns the endpoint associated with serviceID otherwise empty string. | |||
func FetchEndpoints(serviceID string, serviceEndpoint []ServiceEndpoint) string { |
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 see FetchVPCEndpoint can't be used in this because that has a completely a different params and logic, lets mention that here in the comment.
f3ec20c
to
9f2e34b
Compare
Updated document and added UT. |
9f2e34b
to
8544142
Compare
@Karthik-K-N: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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: Karthik-K-N, mkumatag 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 |
What this PR does / why we need it:
Adds support to override service endpoints for cos, transitgateway, resourcecontroller, powervs.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1646
Special notes for your reviewer:
/area provider/ibmcloud
Release note: