Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
OCM-3227: Add support for regionalized urls #876
OCM-3227: Add support for regionalized urls #876
Changes from 3 commits
870fc58
2d834ad
dee9f82
c1116e1
b348018
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
any idea what use cases these alternativeURLs are used for? I can guess at it from their context, just want to make sure we've done some due diligence to make sure they dont also need to be region aware (I dont think they do since they seems like explicit overrides)
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.
Alternative URLs are used to set different URLs for different services. For example if you wanted to run the SDK against integration accounts mgmt and staging clusters service. URL uses Alternative URLs under the hood with an empty identifier (the default URL). Region utilizes this same precedent albeit with an explicit pattern to uphold.
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.
should
region
andurl
be mutually exclusive? they effectively are, with region winning since its set last. just wondering if would be better with an explicit check with a warning/error. or maybe that should be in the end client not the sdk.bit nit picky, not too worried about it, use your best judgement.
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 was thinking the same thing. I don't believe the SDK enforces anything like this today and leaves these decisions up to the consumers.
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.
it's a valid point. because region is now meaningful, then
--region=integration
works exactly as you think. no region is commercial prod.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.
so you connect to "integration" as a
Region
, get ocm-shards.json, to extract the singapore region url and set it withURL
rather thanRegion
...i understand why its done like this as written, but it seems a bit...muddy. Like what real value isConnectionBuilder.Region
providing here besides a slightly different way of setting the url?My initial though with adding region support to ocm-sdk was to reduce duplicate work in downstream consumers. We are basically implementing a service discovery mechanism. Point a client at a global OCM instance, ask it what regions is knows about (ocm-shards.json, which admittedly is a short term WIP concept which will probably change as we work with it in UI and cli tools), and select the right url based on a short human usable string (i.e. "rh-singapore"). We need to implement that same basic thing in both rosa installer and ocm cli.
Does this MR facilitate that workflow?
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, we've already chatted about this a bit but to bring it over here...
This is a byproduct given the current setup of ocm shards. Ideally during the discovery phase we would only care about the region ID and that region ID would directly correlate to the region URL. Such as
api.rh-singapore.openshift.com
. This would reduce some of the "mud" in this example.This MR can facilitate the workflow you are talking about once we settle into our design of the region source-of-truth. Ultimately we are just overwriting the URL based on the desired region. IMO I don't think the SDK should be attempting to auto-discover regions when the Region flag is set, it should just attempt to connect and consumers should dictate how that region is discovered.