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 7 commits intoblake/pr-qualityfrom
Conversation
…oth_branch_and_commit_id_provided
domino-blake
changed the title
|
Once #265 merges, need to change this to target master |
|
The description says "No existing behaviour changes" but there seems to be a few functional changes |
ddl-bira-ignacio
left a comment
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.
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.
| # 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?
| 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
2 participants
What does this PR do?
Adds new backwards-compatible methods and parameters. No existing behaviour changes.
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