-
Notifications
You must be signed in to change notification settings - Fork 16
Conversation
…st_params-to-reference-connector_param-values
…lect the confidential nature of the values
…st_params-to-reference-connector_param-values
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.
nice @galvana, besides adding header support, this saas config iteration adds several other improvements. The query_params spec is especially more readable than before.
if value: | ||
placeholders = re.findall("<(.+?)>", value) | ||
for placeholder in placeholders: | ||
value = value.replace( | ||
f"<{placeholder}>", str(param_values[placeholder]) | ||
) | ||
return value |
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 there be validation when creating/updating the saas config to make sure placeholders have corresponding values in request_params? I'd like to be as helpful as possible when the saas config is being created.
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'll and a logger statement for now and create a separate ticket for validating the different places the placeholders can be used.
- `pagination` An optional strategy used to get the next set of results from APIs with resources spanning multiple pages. Details can be found under [SaaS Pagination](saas_pagination.md). | ||
|
||
## Request params in more detail | ||
The `request_params` list is what provides the values to our various placeholders in the path, headers, query params and body. Values can be `identities` such as email or phone number, `references` to fields in other collections, or `connector_params` which are defined as part of configuring a SaaS connector. Whenever a placeholder is encountered, the placeholder name is looked up in the list of `request_params` and corresponding value is used instead. Here is an example of placeholders being used in various locations: |
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.
Now that query_params
are separated, the name of request_params
isn't intuitive without these docs, it's really where we're specifying request values. Just a personal opinion.
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.
@eastandwestwind brought up the same point this morning, I'm open to suggestions for a better name, some ideas:
- param_value_map
- param_values
- request_param_values
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.
ah good suggestions, I like param_values
, shorter than request_param_values
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.
Thanks for all the efforts to change this
prepared_request: SaaSRequestParams = SaaSRequestParams( | ||
method=HTTPMethod.GET, path=test_request_path, params={}, body=None | ||
method=test_request.method, path=test_request.path | ||
) | ||
self.client().send(prepared_request) | ||
return ConnectionTestStatus.succeeded |
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.
Unrelated to these changes, in testing this, if the user hasn't set their secrets, self.secrets
would be None, and therefore we couldn't do f"{self.client_config.protocol}://{self.secrets[host_key]}"
and would get a weird typeerror, i'd expect them to be given more detailed information that their secrets weren't configured.
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.
This looks good to me, thanks for being thorough here, I don't want to hold up the merge because of the unrelated piece ^
Adding the ability to specify headers in requests of a SaaS config, plus changes to specifying query_params and param_values. Co-authored-by: Adrian Galvan <[email protected]>
Purpose
Adding the ability to specify headers in requests of a SaaS config.
Changes
headers
andquery_params
torequest
in thesaas_config
.type
param fromrequest_params
, each value in a request param is free to be used anywhere in the placeholders of path, headers, query_params, and body.saas_query_config
by refactoring the logic to be able to use a sharedmap_param_values
function.Checklist
Run Unsafe PR Checks
label has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #289