direct: Fix jobs and pipelines RemoteType to be superset of StateType#4443
Open
direct: Fix jobs and pipelines RemoteType to be superset of StateType#4443
Conversation
Collaborator
|
Commit: 17aa0f1
19 interesting tests: 7 KNOWN, 5 SKIP, 5 RECOVERED, 2 flaky
Top 50 slowest tests (at least 2 minutes):
|
4a33554 to
e750721
Compare
7212650 to
60b8dbb
Compare
2df42b4 to
ca1a7e5
Compare
Create JobRemote and PipelineRemote wrapper structs that embed the StateType (JobSettings and CreatePipeline respectively) so that all paths in StateType are valid paths in RemoteType. This ensures JSON plan paths work correctly on both types. The wrapper structs include: - All fields from StateType via embedding - Remote-specific fields (job_id, created_time, state, etc.) - Custom MarshalJSON/UnmarshalJSON methods to properly serialize embedded struct fields alongside additional fields Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
ca1a7e5 to
c85e127
Compare
shreyas-goenka
approved these changes
Feb 5, 2026
pietern
reviewed
Feb 6, 2026
| "myjob_id": "[NUMID]", | ||
| "myjob_name": "Test Migration Job", | ||
| "myjob_timeout": "", | ||
| "myjob_timeout": "0", |
Contributor
There was a problem hiding this comment.
Ah, I see this wasn't resolved before, but now it is.
| "action": "update", | ||
| "old": "0", | ||
| "new": "${resources.jobs.test_job.timeout_seconds}", | ||
| "remote": "0" |
Contributor
There was a problem hiding this comment.
Will this downgrade during deploy?
Contributor
Author
There was a problem hiding this comment.
We don't do any downgrades (yet).
| "storage": "dbfs:/pipelines/[FOO_ID]" | ||
| }, | ||
| "changes": { | ||
| "id": { |
Contributor
There was a problem hiding this comment.
It's weird to see this both special cased and part of the response payload.
Contributor
Author
There was a problem hiding this comment.
I agree. I'd get rid of this field but it comes from embedded CreatePipeline.
We can instruct framework to drop this change from change list. We can have special reason in the config, e.g. "!drop" which causes change to disappear. Might impede debugging though.
| EffectiveUsagePolicyId string `json:"effective_usage_policy_id,omitempty"` | ||
| JobId int64 `json:"job_id,omitempty"` | ||
| RunAsUserName string `json:"run_as_user_name,omitempty"` | ||
| TriggerState *jobs.TriggerStateProto `json:"trigger_state,omitempty"` |
Contributor
There was a problem hiding this comment.
How can we ensure this remains up to date?
Contributor
Author
There was a problem hiding this comment.
We could write a custom test that uses reflection to check that JobRemote & jobs.Job fields match.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Create JobRemote and PipelineRemote wrapper structs that embed the StateType (JobSettings and CreatePipeline respectively) so that most paths in StateType are valid paths in RemoteType. This ensures JSON plan paths work correctly on both types.
The wrapper structs include:
Also include special "!drop" reason for id field on pipeline since it overlaps with builtin "id" and it output-only.
Why
See #4442 for explanation
Tests
Additional test for new types that checks that tracks SDK types and will catch a case when new fields are added to SDK that need to be added to custom types.