DataSource: Use the same struct for both Add+Update commands #88018
Conversation
This file contains 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
ryantxu
requested review from
evictorero,
AgnesToulet,
wbrowne,
marefr,
xnyo,
jtheory,
rwwiv,
JacobsonMT,
yuri-tceretian,
grobinson-grafana and
owensmallwood
and removed request for
a team
ryantxu
added
no-backport
ryantxu
commented
May 17, 2024
| @@ -4645,6 +4597,58 @@ | |||
| }, | |||
| "type": "object" | |||
| }, | |||
| "DataSourceCommand": { | |||
There was a problem hiding this comment.
really just a name change for the command object
There was a problem hiding this comment.
The internal changes look fine to me, but could we still specify separate swagger definitions for adding and updating? Firstly, it helps to make it clearer for users which fields can be specified for which operation. Secondly, it means that we don't introduce a breaking change for Terraform where we use the current commands (DS resource in Terraform).
|
Con: It makes things less explicit/harder for devs to understand what fields to provide for a create vs update and easier to introduce unexpected bugs. |
|
Closing this in favor of #88036 -- the syntax is annoying, but I think leads to a better solution |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
See an alternative approach #88036
In #87718 -- we need to duplicate the validation functions because the commands for Add/Update are almost identical -- but not identical.
An alternative approach would be to make a base class for the common properties and extend the explicit ones for each -- but that seems like more work than it is worth.
The difference is that Update has a version and internal ID -- and Add has a UserID 馃し馃徎