new API additions — branch param, app_id, files_download, bug fixes#266
new API additions — branch param, app_id, files_download, bug fixes#266domino-blake wants to merge 12 commits into
Conversation
|
The description says "No existing behaviour changes" but there seems to be a few functional changes |
ddl-bira-ignacio
left a comment
There was a problem hiding this comment.
The description says "No existing behaviour changes", but the diff also includes param renames and deprecation shims.
| self.get_hardware_tier_id_from_name(hardware_tier_name) | ||
| if hardware_tier_name | ||
| else None | ||
| resolved_hardware_tier_id = ( |
There was a problem hiding this comment.
issue: Looks like this now calls get_hardware_tier_id_from_name even when no tier name was passed. I think we need to keep this conditional
| return self._get(url) | ||
|
|
||
| def get_hardware_tier_id_from_name(self, hardware_tier_name: str): | ||
| def get_hardware_tier_id_from_name(self, hardware_tier_name: Optional[str]): |
There was a problem hiding this comment.
suggestion: given hardware_tier_name is used here, I don't think it should be made optional.
There was a problem hiding this comment.
Reverted to hardware_tier_name: str. With the call-site conditional restored in the previous commit, this function is never reached with None, so the original signature is correct.
There was a problem hiding this comment.
issue: I see deprecation shims going in across a bunch of methods here, but the tests in this PR are focused on the new features. Would be great to either add shim-specific tests here or split that work into its own PR so coverage stays tight.
There was a problem hiding this comment.
Some shim work did land here alongside the new features rather than living cleanly in PR #267:
- app_publish / app_unpublish shims (commitId → commit_id, appId → app_id): these are coupled to PR B's new branch parameter on app_publish. Adding branch while leaving the adjacent params in camelCase would have shipped a mixed-casing API, so the renames came with the feature.
- runs_start, run_stop, runs_status, get_run_log, runs_stdout, files_list, endpoint_publish shims: you're right that these are unrelated to PR B's scope and leaked in via a "cherry pick" commit that bundled too much. They're also present on PR rename camelCase params to snake_case with deprecation shims #267 (which is the rename PR), so they'll be deduplicated cleanly when PR rename camelCase params to snake_case with deprecation shims #267 merges after this one.
If you'd prefer, I can either:
- Strip the unrelated shims from this PR (would require rewriting history on commit 04e72a7 and the downstream commits that depend on it), or
- Add shim coverage to this PR's test file for the ones that touch PR B's APIs (app_publish / app_unpublish), and let PR rename camelCase params to snake_case with deprecation shims #267 cover the rest.
| # This will fetch app_id of app in current project | ||
| @property | ||
| def _app_id(self): | ||
| def app_id(self) -> Optional[str]: |
There was a problem hiding this comment.
issue: _app_id has been around for a while and some folks might be using it even though it's private. Would it make sense to keep _app_id as a deprecated alias that points to app_id, at least for one release?
There was a problem hiding this comment.
Added _app_id as a deprecated alias property that issues a DeprecationWarning and forwards to app_id. Existing callers will keep working for at least one release. 016ab96
| def _validate_hardware_tier_id(self, hardware_tier_id: str) -> bool: | ||
| def _validate_hardware_tier_id(self, hardware_tier_id: Union[str, Dict]) -> bool: | ||
| if isinstance(hardware_tier_id, dict): | ||
| hardware_tier_id = hardware_tier_id.get("value", hardware_tier_id) |
There was a problem hiding this comment.
question: If the dict doesn't have a "value" key, .get just returns the original dict and the tier comparison will never match. Should we raise a clear error here instead of letting it fall through to "not found"?
| :return: Raw file content (urllib3 response stream). | ||
| """ | ||
| if commit_id is None: | ||
| commit_id = self.commits_list()[0]["id"] |
There was a problem hiding this comment.
suggestion: Add a guard to commits_list in case it returns empty
There was a problem hiding this comment.
Added. commits_list() now gets stored, checked for empty, and a ValueError is raised b4b90d9
…oth_branch_and_commit_id_provided
5bde7ba to
4140ce3
Compare
What does this PR do?
Adds new backwards-compatible methods and parameters, plus one bug fix to
_validate_hardware_tier_id. The only existing behaviour change is intentional:_validate_hardware_tier_idnow accepts dict input fromcompute_cluster_propertiesinstead of raisingHardwareTierNotFoundExceptionfor a valid tier (#174).A small number of
app_publish/app_unpublishdeprecation shims (commitId/appId) also land here because they're coupled to the new branch parameter see inline reply for context. Broader rename work lives in PR #267.New features
job_start(branch=...)— launch a job from a named branch without constructing amainRepoGitRefdict manually. Mutually exclusive withcommit_id. Closes Request: Allow job_start to specify the job's git repo branch to run on #231.app_idproperty — previously private (_app_id), now a public read-only propertyreturning the current project's app ID. Closes Can we make Domino._app_id a public API? #127.
app_get_status(app_id)— previously private (__app_get_status), now publiclyaccessible. Closes Can we make Domino._Domino__app_get_status(app_id) a public API #128.
files_download(path, commit_id=None)— convenience wrapper overblobs_get_v2that defaults to the latest commit, eliminating the need to call
commits_list()manually. Closes Make a method to download a file without having to get the blob id first #24, Blob Get By File Name #31.
Bug fix
_validate_hardware_tier_idnow handles dict input (e.g.{"value": "small-k8s"})from
compute_cluster_properties, which previously causedHardwareTierNotFoundExceptioneven when the tier existed. Closes BUG: in _validate_hardware_tier_id #174.
Docs
runs_startvsjob_startcomparison table to README.md and README.adoc. Closes Document The Differnce Between domino.runs_start() and domino.job_start() #122.branchparam in both READMEs.Issues closed
#24, #31, #122, #127, #128, #174, #231
Testing
New tests added for every change:
tests/test_jobs.py— branch param, hardware tier dict inputtests/test_app.py—app_get_statustests/test_apps.py—app_idpropertytests/test_files.py—files_downloadUnit tests passing
All changes are backwards compatible
Documentation updated