-
Notifications
You must be signed in to change notification settings - Fork 122
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
[Internal] Update Jobs GetRun API to support paginated responses for jobs and ForEach tasks with >100 runs #725
base: main
Are you sure you want to change the base?
Conversation
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.
Overall LGTM, just a few small nits about style & test cases. The main question I have is whether we should add an extra documentation comment for this method.
*, | ||
include_history: Optional[bool] = None, | ||
include_resolved_values: Optional[bool] = None, | ||
page_token: Optional[str] = None) -> jobs.Run: |
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 we have a doccomment here explaining that this makes multiple requests to return all tasks/iterations when the underlying has more than 100?
requests_mock.get(make_path_pattern(1337, "tokenToThirdPage"), text=json.dumps(run3)) | ||
w = WorkspaceClient(config=config) | ||
|
||
run = w.jobs.get_run(1337, page_token="initialToken") |
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.
Will users ever specify an initial token?
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.
At this point users cannot get initial page token from any SDK function. So this parameter is redundant. The function that is overloaded is generated from openApi spec and it also comes with the page_token param. So I assumed that it's ok to repeat the same function contract in the mixin as well.
I see two paths:
- we hide the page_token param in the mixin and make our change opaque to the users
- we do not hide this parameter and stay consistent with the openApi spec for this spec and for the other functions that use page_token. Maybe in the future the initial token will be exposed to the users 🤷
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.
Discussed offline. Let's leave the parameter there so the mixin matches the underlying API. We'll clean up token parameters separately and can remove it here when we do that.
requests_mock.get(make_path_pattern(1337, "tokenToThirdPage"), text=json.dumps(run3)) | ||
w = WorkspaceClient(config=config) | ||
|
||
run = w.jobs.get_run(1337, page_token="initialToken") |
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.
ditto about the initial token?
…kiko10/JOBS-19305
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
Changes
Jobs GetRun 2.2 behind the scenes pagination
Tests
make test
run locallymake fmt
applied