Skip to content

fix(experiment): remove deprecated job_id parameter from monitor_progress() (HYBIM-931) - #194

Open
etserend wants to merge 2 commits into
mainfrom
fix/HYBIM-931-remove-job-id-from-monitor-progress
Open

fix(experiment): remove deprecated job_id parameter from monitor_progress() (HYBIM-931)#194
etserend wants to merge 2 commits into
mainfrom
fix/HYBIM-931-remove-job-id-from-monitor-progress

Conversation

@etserend

Copy link
Copy Markdown
Contributor

Summary

  • Removes job_id: str | None = None keyword argument from Experiment.monitor_progress() signature
  • Removes the associated DeprecationWarning block
  • Removes the job_id docstring entry
  • Removes now-unused import warnings
  • Deletes test_deprecated_job_id_warns test
  • Adds breaking change entry to CHANGELOG.md

Background

job_id was deprecated in commit cbdfe22 (HYBIM-882 / PR #109) when the jobs table was retired. Progress is now tracked directly via experiment status. This removes the parameter after the deprecation window has elapsed. Requested by @adityamehra in PR #148 review.

Breaking Change

Callers passing job_id= to monitor_progress() must remove that argument.

Out of scope

src/splunk_ao/resources/models/create_job_request.py and create_job_response.py — these are auto-generated from openapi.yaml and contain a job_id field for the /jobs API endpoint, which is unrelated to this parameter.

Test plan

  • test_deprecated_job_id_warns deleted — parameter no longer exists
  • Remaining 7 tests in test_experiment_progress.py all pass
  • 1901 tests pass in full suite
  • grep confirms no external callers pass job_id= to monitor_progress() anywhere in the repo

Jira: HYBIM-931

🤖 Generated with Claude Code

@fercor-cisco fercor-cisco left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 This review was generated by the Astra agent (claude-opus-4-8). It may contain mistakes.

Verdict: needs_discussion — Code change is correct and complete; the only open item is confirming the deprecation window is adequate, which HYBIM-931 explicitly requires before merge.

General Comments

  • 🟠 major (question): The deprecation window looks very short. Per HYBIM-931, job_id was deprecated on 2026-07-24 in commit cbdfe22 (PR #109), and this removal is dated 2026-07-31 — roughly 7 days later. The ticket explicitly states "Once an adequate deprecation window has elapsed, fully remove the parameter" and "Confirm the deprecation window is sufficient before merging." Two things to clarify before merge: (1) Was the job_id deprecation ever shipped in a released version, or is it still under ## Unreleased in the CHANGELOG? If the deprecation warning never reached a public release, external callers never got a warning cycle at all, and the one-week gap is effectively zero. (2) If it was released, is one week considered an adequate window for this SDK's compatibility policy? This is the only thing gating my approval — the code change itself is correct.

@fercor-cisco

Copy link
Copy Markdown
Collaborator

Confirmed safe to merge — the deprecation-window concern from my earlier review is moot:

  • job_id was a pure no-op. Since cbdfe22 (HYBIM-882) the parameter was referenced only inside the DeprecationWarning block; no logic in monitor_progress() ever read it. Progress is tracked via get_status() polling.
  • The deprecation never shipped. Both the deprecation (cbdfe22) and this removal sit under ## Unreleased in the CHANGELOG (last release: v0.10.0). No public release ever exposed the parameter or emitted the warning, so there's no real deprecation window to satisfy.
  • Combined with splunk-ao being a new SDK intentionally making breaking changes vs. the old galileo SDK, there's no back-compat obligation here. Breaking change is correctly documented in the CHANGELOG.

👍 from me.

fercor-cisco
fercor-cisco previously approved these changes Jul 31, 2026
…ress() (HYBIM-931)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@etserend
etserend force-pushed the fix/HYBIM-931-remove-job-id-from-monitor-progress branch from fa7be62 to 71eefe5 Compare July 31, 2026 20:29

@fercor-cisco fercor-cisco left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is bringing CHANGELOG.md which is out of scope for this ticket.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants