-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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
DataSource: Move the shared parts of Add+Update commands into a base struct #88036
base: main
Are you sure you want to change the base?
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.
My +1 for this approach. It's annoying that you need to specify BaseDatasource
everytime but I prefer that than both commands sharing the same struct.
pkg/services/datasources/models.go
Outdated
type AddDataSourceCommand struct { | ||
// The input values shared with both Add+Update commands (also part of the DTO) | ||
// This part of the command can share validation between add and update actions | ||
type BaseDataSourceCommand struct { |
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.
the name may be a bit confusing because it could mean that DeleteDatasource
also extends this one. Maybe something like BaseWriteDataSourceCommand
?
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.
In favor of this proposal as well. The trade-off with having to instantiate BaseDataSourceCommand for create/update is worth it compared to the non-explicit nature of the other proposal.
pkg/services/datasources/models.go
Outdated
ReadOnly bool `json:"-"` | ||
EncryptedSecureJsonData map[string][]byte `json:"-"` | ||
UpdateSecretFn UpdateSecretFn `json:"-"` | ||
} | ||
|
||
// Also acts as api DTO | ||
type AddDataSourceCommand struct { | ||
BaseDataSourceCommand |
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.
Nit. Does this need json inline tag to make the JSON unmarshal work as expected?
pkg/services/datasources/models.go
Outdated
APIVersion string `json:"apiVersion"` | ||
// swagger:ignore | ||
IsPrunable bool | ||
BaseDataSourceCommand |
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.
Nit. Does this need json inline tag to make the JSON unmarshal work as expected?
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
This is an alternative approach to #88018
Rather than unify the two commands, this puts the shared parts into a base struct.
This adds a lot more typing when creating commands, but at least makes the difference between the two commands more clear 🤷🏻