fix(trainer): report a missing TrainJob as not found instead of unowned - #158
fix(trainer): report a missing TrainJob as not found instead of unowned#158Krishna-kg732 wants to merge 1 commit into
Conversation
Signed-off-by: krishna-kg732 <krishnagupta.kg2k6@gmail.com>
google-oss-prow
Bot
added
kind/bug
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/ok-to-test |
|
Thanks @Krishna-kg732, Nice catch and a clean fix,.. I guess the persona-dependent answer for a mistyped name was actively misleading agents. This is exactly what we needed 🎉 |
| ("update", lambda: update_training_job("external", "suspend"), "delete_job"), | ||
| ], | ||
| ) | ||
| def test_non_admin_blocked_from_mutating_external_job(tool, call, attr): |
There was a problem hiding this comment.
Nit: the update row still passes attr="delete_job", but update_training_job never calls client.delete_job.. it just patches via the custom-objects API. The patch assert below already covers it..
either drop attr for the update case or only assert delete_job on the delete row.
| ): | ||
| result = call() | ||
|
|
||
| assert result["error_code"] == "RESOURCE_NOT_FOUND" |
There was a problem hiding this comment.
Nit: prefer RESOURCE_NOT_FOUND / SDK_ERROR from tests.common (same as VALIDATION_ERROR already imported) instead of raw strings.
|
@Krishna-kg732 are you still working on this ? Did you get time to resolve nit picks raised ? |
Description
is_mcp_managedreturnedFalsefor a TrainJob that does not exist, the same value it returns for one that exists without the MCP ownership label.delete_training_jobandupdate_training_jobgate on ownership before existence, so a mistyped name was reported asVALIDATION_ERROR: "was not created by MCP"with a permissions hint, instead of not found.platform-adminskips the gate and correctly gotRESOURCE_NOT_FOUND, while every other persona got the misleading error.get_trainer_ownership, returningmanaged,unmanagedormissing. Missing now maps toRESOURCE_NOT_FOUND, and the ownership message is reserved for jobs that actually exist._require_mcp_ownershiphelper, removing duplicated error construction that had already drifted between the two tools.No behaviour change for
managed,unmanagedor API-failure outcomes.Related Issue
No existing issue. Found while exercising the Katib optimizer client against a live cluster.
Checklist
make test-python)make verify)git commit -s)Testing
Unit tests
Integration tests
E2E tests
Manually tested (describe below)
Added
test_missing_job_end_to_end_from_a_real_404, which mocks only the Kubernetes API so the real ownership helper runs. The other ownership tests stub the helper, so they pin the gate's branching but cannot catch this class of regression.Verified the coverage actually bites: reintroducing the bug fails 3 tests, and all pass with the fix.
Reproduced the original behaviour first by driving both tools with a mocked 404 across all three personas.
Fills two existing
TODO(test)markers intrainer/api/lifecycle_test.py.make test-python: 485 passed.make verify: all checks passed./kind bug
/area trainer