-
Notifications
You must be signed in to change notification settings - Fork 24
[CIVIS-11467] civis-python: add support to civis.utils.run_template for passing remote_host_id and credential_id arguments #526
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
Conversation
…t tests accordingly. Handle case where JSONValue and return_as conflict.
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.
Pull Request Overview
This PR adds support for passing additional arguments to civis.utils.run_template and introduces a new return_as parameter to control the return type. The legacy JSONValue boolean parameter is deprecated in favor of the more flexible return_as string parameter, which supports three options: "files" (default), "JSONValue", and "future". This enables using run_template with SQL template scripts that require remote_host_id and credential_id arguments.
Key Changes:
- Added
**kwargsparameter torun_templateto pass additional arguments topost_custom - Introduced
return_asparameter with three options: "files", "JSONValue", and "future" - Deprecated the
JSONValueboolean parameter with backward compatibility support - Added comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/civis/utils/_jobs.py |
Added return_as parameter, **kwargs support, and deprecation handling for JSONValue |
tests/test_jobs.py |
Added tests for return_as parameter options, kwargs passing, and deprecation warnings |
CHANGELOG.md |
Documented the changes to run_template |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…)... ...and cleanup whitespace Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This reverts commit 31d5d4e.
jacksonlee-civis
left a comment
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 is looking great! Once the way we handle the deprecated JSONValue kwarg and the validation of return_as is updated, this should be good to go.
Co-authored-by: Jackson Lee <86482098+jacksonlee-civis@users.noreply.github.com>
| "As of civis-python v2.8.0, civis.utils.run_template can return three " | ||
| "types of values, so 'JSONValue' is deprecated " |
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.
Trying to clarify a bit more since we're making a potentially confusing change (deprecating an argument named JSONValue, while we expect the string "JSONValue" to be one of the allowed values for the new arg return_as),
| "As of civis-python v2.8.0, civis.utils.run_template can return three " | |
| "types of values, so 'JSONValue' is deprecated " | |
| "As of civis-python v2.8.0, the boolean argument 'JSONValue' of " | |
| civis.utils.run_template is deprecated " |
Co-authored-by: Jackson Lee <86482098+jacksonlee-civis@users.noreply.github.com>
jacksonlee-civis
left a comment
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!
The function run_template in _jobs.py has been updated:
CHANGELOG.mdat the repo's root level