Uh oh!
There was an error while loading. Please reload this page.
Add isLocal parameter to FieldTriggers and ClassifyChanges - #3741
Conversation
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| } | ||
| func (r *ResourceMlflowModel) FieldTriggersLocal() map[string]deployplan.ActionType { |
There was a problem hiding this comment.
Looking at this boilerplate in every resource I wonder if we should keep FieldTriggers() support in adapter? and allow implementations define one of these configurations:
- FieldTriggers
- FieldTriggersLocal & FieldTriggersRemote
Other mixes (e.g. FieldTriggers and FieldTriggersLocal) should be rejected by validate().
There was a problem hiding this comment.
I think either approach is reasonable.
I expect us to have more resources where the "etag" diff matters, like genie spaces. The advantage with the current method is it makes it clear how local and remote diffs are being computed.
There was a problem hiding this comment.
In general, I prefer one way to do things when possible.
There was a problem hiding this comment.
Regarding one way of doing things, perhaps we should have one function and pass a boolean isLocal to it?
FieldTriggers(isLocal bool)
ClassifyChanges(isLocal bool)
Most resources would just ignore the parame
FieldTriggers(_ bool) {} ClassifyChanges(_ bool) {}
This would be one way of doing things, simplify adapter.go and have minimal boilerplate for resources that don't have a difference.
This would also allow to structure code in a way where local and remote share implementation.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| } | ||
| func (r *ResourceMlflowModel) FieldTriggersLocal() map[string]deployplan.ActionType { |
There was a problem hiding this comment.
Regarding one way of doing things, perhaps we should have one function and pass a boolean isLocal to it?
FieldTriggers(isLocal bool)
ClassifyChanges(isLocal bool)
Most resources would just ignore the parame
FieldTriggers(_ bool) {} ClassifyChanges(_ bool) {}
This would be one way of doing things, simplify adapter.go and have minimal boilerplate for resources that don't have a difference.
This would also allow to structure code in a way where local and remote share implementation.
denik
left a comment
There was a problem hiding this comment.
Change looks good, just have a suggestion regarding docstring.
Uh oh!
There was an error while loading. Please reload this page.
| // Note, Create/Delete are handled explicitly and never compared. | ||
| const ( | ||
| ActionTypeUnset ActionType = iota | ||
| ActionTypeUndefined ActionType = iota |
There was a problem hiding this comment.
This would be nice to call out in PR description.
Uh oh!
There was an error while loading. Please reload this page.
…ange This merge incorporates changes from PR #3741 which adds an isLocal parameter to FieldTriggers and ClassifyChange methods to differentiate between local changes (user modifications) and remote changes (drift detection). Key changes: - Renamed ActionTypeUnset to ActionTypeUndefined - Updated FieldTriggers to accept isLocal bool parameter - Updated ClassifyChange to accept isLocal bool parameter - Consolidated dashboard's separate FieldTriggersLocal/Remote methods into single FieldTriggers(isLocal bool) method - Updated adapter to store separate trigger maps for local and remote 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
## Changes: 1. Change ActionTypeUnset to ActionTypeUndefined 2. Add isLocal parameter to FieldTriggers and ClassifyChanges ## Why Resources like dashboards need to have differentiated local and remote diff computations. For example, the `serialized_dashboard` field should only be compared in local diff, and `etag` should only be compared in remote drift. Thus we need different triggers based on whether the execution is local or remote. ## Tests Existing tests.
Changes:
Why
Resources like dashboards need to have differentiated local and remote diff computations. For example, the
serialized_dashboardfield should only be compared in local diff, andetagshould only be compared in remote drift.Thus we need different triggers based on whether the execution is local or remote.
Tests
Existing tests.