[RAPTOR-19349] add offset to workload/artifact list - #784
Conversation
|
🎫 Jira: |
|
👋 Thanks so much for contributing to the DataRobot community! As a quick heads-up on how our team handles reviews: if you're still iterating on Once everything is finalized and you're ready for feedback, just click "Ready for review" |
Code OwnershipCli Maintainers
Workload Cli
Review requested from the teams above. Labels will be removed automatically upon approval. |
|
/approve-smoke-tests |
|
🔐 Fork PR smoke tests triggered by @ajalon1 What happens next:
|
|
🔐 Fork smoke tests started by maintainer ⏳ Security scans passed. Running smoke tests... Commit: |
|
✅ All smoke tests passed! (Fork PR) ✅ Security Scan: success |
There was a problem hiding this comment.
Hey @ScottOglesby 👋🏽
Pretty good. I've got a lot of small nits and observations here, that are really just stretch goals, that are coming up as we're maturing this codebase. I just like making general suggestions.
I would approve after you update ListArtifacts in the internal API module to validate limit and offset. Anything else is up to the the Workload team.
|
|
||
| workload.AddStatusFlag(cmd, &status) | ||
| cmd.Flags().IntVar(&limit, "limit", 100, "Maximum number of artifacts to return") | ||
| cmd.Flags().IntVar(&offset, "offset", 0, "Number of artifacts to skip before returning results") |
There was a problem hiding this comment.
Probably not your intention, but I'm happy to see that this is backwards-compatible. Not a breaking change. 😆
| @@ -410,7 +410,7 @@ func TestListWorkloads_FollowsNextAndTruncatesToLimit(t *testing.T) { | |||
|
|
|||
| func TestListWorkloads_RejectsNonPositiveLimit(t *testing.T) { | |||
There was a problem hiding this comment.
Add a TestListWorkloads_RejectsNegativeOffset() test
| func ListArtifacts(limit int, status Status) ([]Artifact, error) { | ||
| endpoint := "/api/v2/artifacts/?limit=" + strconv.Itoa(limit) | ||
| func ListArtifacts(limit, offset int, status Status) ([]Artifact, error) { | ||
| endpoint := "/api/v2/artifacts/?limit=" + strconv.Itoa(limit) + "&offset=" + strconv.Itoa(offset) |
There was a problem hiding this comment.
Could consider switching this over to use url.Values{} like ListWorkloads() does as that would be a better pattern than building this by hand.
Absolutely not your problem. If you do tackle it I'd suggest doing this in a followup PR.
Something closer to this:
query := url.Values{}
query.Set("limit", strconv.Itoa(limit))
query.Set("offset", strconv.Itoa(offset))
if status != "" {
query.Set("status", string(status))
}
pageURL, err := config.GetEndpointURL("/api/v2/artifacts/")| func ListWorkloads(limit, offset int, statuses []string) ([]Workload, error) { | ||
| if limit <= 0 { | ||
| return nil, fmt.Errorf("invalid limit %d: must be positive", limit) | ||
| } | ||
|
|
||
| if offset < 0 { | ||
| return nil, fmt.Errorf("invalid offset %d: must be non-negative", offset) | ||
| } | ||
|
|
There was a problem hiding this comment.
I think ListArtifacts() in the internal module should validate limit and offset the way ListWorkloads() does.
| if offset < 0 { | ||
| return fmt.Errorf("invalid --offset %d: must be non-negative", offset) | ||
| } |
There was a problem hiding this comment.
Another suggestion for a followup PR would be refactoring this to a custom pflag.Value. We do this already in cmd/internal/pollflags/pollflags.go.
Absolutely Not blocking, since the current if-return-error is widespread throughout the repo, but would be a nice update. Would also have the benefit of moving the validation from runtime to parsetime.
2 participants
RATIONALE
Add
--offsetto the list commands for artifacts and workloads.--limitalready exists, but not having--offsetmade listing items beyond the first page impossible.CHANGES
--offsetto the list commands for artifacts and workloadsTesting
Ran
task lint,task testPR Automation
Comment-Commands: Trigger CI by commenting on the PR:
/trigger-smoke-testor/trigger-test-smoke- Run smoke tests/trigger-install-testor/trigger-test-install- Run installation testsLabels: Apply labels to trigger workflows:
run-smoke-testsorgo- Run smoke tests on demand (only works for non-forked PRs)Important
For Forked PRs: The
run-smoke-testslabel won't work. A required Smoke Tests check will block merge until a maintainer acts:/approve-smoke-teststo run smoke tests (results will set the check)/skip-smoke-teststo bypass the check without running testsPlease comment requesting a maintainer review if you need smoke tests to run.
Note
Low Risk
Backward-compatible CLI pagination wired to existing list APIs; default offset preserves prior behavior and changes are limited to list/query plumbing.
Overview
Adds
--offset(default0) todr workload listanddr artifact listso users can skip past the first page of results alongside existing--limit.The list commands validate that offset is non-negative, pass offset into
ListWorkloadsandListArtifacts, and include it in telemetry.ListWorkloadsalso rejects negative offset before building the API query; artifact listing appendsoffsetto the artifacts API URL. Internal callers that only need the first page (deploy name-conflict lookup, workload config wizard picker) now pass0for offset. Tests cover invalid--offseton both commands and updatedListWorkloadscall signatures.Reviewed by Cursor Bugbot for commit 1799239. Configure here.