-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DataSourceRef: Support legacy string parsing when used in an embedded struct. #981
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with questions
ref := &DataSourceRef{} | ||
err = json.Unmarshal([]byte(`"aaa"`), ref) // string as reference | ||
require.NoError(t, err) | ||
require.Equal(t, "aaa", ref.UID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So ds name is moved into Ref.uid? No longer possible to know if there's a name or a uid I guess. Do we need to be able to distinguish/handle this legacy case? Can we fail instead getting this legacy format? Hard to know where these legacy forms comes from, imagine it should be migrated in dashboards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes they should be migrated! but what do we do when they are not? Currently the logic to look up by UID will fallback to lookup by name, so this works OK.
Currently dashboard frontend always does this migration, the place we see issues is in very old alert definitions.
@ryantxu The changes look good. I am a bit late to review. When we have this in |
We already have this in grafana/grafana |
Ah I see, It was in v0.229.0. Thank you. |
@itsmylife -- the changes in grafana/grafana#87224 are no longer required, but are fine to keep (it really just skipped reading properties that were immediately ignored). |
@ryantxu thanks for the info. Then, I'll keep it. |
See: grafana/grafana#87224 -- this "fixed" the issue by avoiding the ref alltogether, but this PR will make it work even when embedded.
This PR moves the special legacy string|object handling into the DataSourceRef code rather than in DataQuery. This means the legacy mode will be supported even when the ref is added as embedded fields.