fix/Address server_url breaking change #226
Conversation
awalker4
changed the title
| base_url = operations.CREATE_DESTINATION_SERVERS[ | ||
| client_url, *_ = self.sdk_configuration.get_server_details() | ||
|
|
||
| base_url = choose_server_url( |
There was a problem hiding this comment.
I'm not sure if I get why do we need to overwrite the platform-specific operations? As I understand that it didn't break anything yet and users can only use our prod environment, hardcoded in each operation.
The only thing we want to overwrite is partition operation which got broken.
I'd limit the changes only to that operation - it's rather "frozen" while we'll be working on platform-specific operations and might need regenerating these files. WDYT?
There was a problem hiding this comment.
Ah, that's an excellent point!
| if not base_url: | ||
| return "" | ||
| # -- add a url scheme if not present (urllib.parse does not work reliably without it) | ||
| if "http" not in base_url: |
There was a problem hiding this comment.
I would parse the URL first and then check if a url has a scheme by using parsed_url.scheme. Then we can prepend http:// or https://
There was a problem hiding this comment.
That comment has been around for a while - I tried parsing without the scheme and it does look like we need to make sure http:// is there first or it gets confused.
In [2]: urlparse("localhost:8000/general/v0/general")
Out[2]: ParseResult(scheme='localhost', netloc='', path='8000/general/v0/general', params='', query='', fragment='')
| parsed_url: ParseResult = urlparse(base_url) | ||
| # First, see if the endpoint has a url: | ||
| # s.general.partition(server_url=...) | ||
| if endpoint_url is not None: |
There was a problem hiding this comment.
This means the endpoint_url is treated as an optional kwarg and it's currently not.
There was a problem hiding this comment.
This can come in from the generated code as None over here
| # -- add a url scheme if not present (urllib.parse does not work reliably without it) | ||
| if "http" not in base_url: | ||
| base_url = "http://" + base_url | ||
| def choose_server_url(endpoint_url, client_url, default_endpoint_url) -> str: |
There was a problem hiding this comment.
I feel like this entire helper function can be written as a oneliner:
url = endpoint_url if endpoint_url is not None else (client_url or default_endpoint_url)
the logic and reasoning behind this can be explained in the docstring.
In order to add the platform-api functions, we now need to handle two openapi specs. When these are merged, we get a breaking change with the
server_urlarg:We published the 0.30.0 client with this change, but let's do the right thing before it breaks a whole lot of users. For now, let's directly edit the endpoint functions and make sure the client level url is still accepted. We can work with Speakeasy to roll this back into the auto generated SDK.
Changes
clean_server_urlhook file to select the right url from three options: the endpoint, the client, or the default from the configtest_server_urls.pyto test every URL case against every endpoint. The user can pass the URL in either location, and the SDK will strip the path if it's given, for backwards compat..genignore. This will unblock the generate action for now, and our test suite will tell us if these files have gotten stale.Other:
workflow.yamlto reorder the openapi specs. Both specs includeHttpValidationError, and the later spec will overwrite the duplicate. This schema changed a bit in Serverless, so some tests are failing on main. Note that themake client-generate-sdkdoes the right thing, but this got reverted when the github generate action ran again.Testing