-
Notifications
You must be signed in to change notification settings - Fork 215
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
docs: add decision record about optional params in CatalogRequest #4325
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #4325 +/- ##
==========================================
+ Coverage 71.74% 75.21% +3.46%
==========================================
Files 919 1053 +134
Lines 18457 21135 +2678
Branches 1037 1184 +147
==========================================
+ Hits 13242 15896 +2654
+ Misses 4756 4722 -34
- Partials 459 517 +58 ☔ View full report in Codecov by Sentry. |
"querySpec": { | ||
//... | ||
}, | ||
"parameters": { |
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.
is this parameters
really needed? additionalScopes
could stay at the root level, as CatalogRequest
itself can already be considered "extensible" (querySpec
is not mandatory so in fact it represent an object extension)
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.
it's not strictly needed, but it would fit the paradigm we often use. Also, if we have this container object, we could easily add other features in the future without changing the shape of the body (i.e. the CatalogRequest
object)
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.
But we'd need to change the shape of parameters
then, the problem is not solved, it's just moved
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.
it would be an extensible map, so we wouldn't need to change it. only whoever consumes the CatalogRequest
objects needs to know to call get(newParamterKey)
. The transformers and the API could remain unchanged.
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.
i think we are getting too technical for this scope, but nothing prevents extensible properties to be at the root level of an object (e.g. Policy.extensibleProperties
, I know, they are not really used, but they are deserialized from the root level of a policy)
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.
if we use a parameters
object, we only need to change the transformers and the API once, and the problem is solved for all future parameters.
If we decide to remove the additionalScopes
later, we could simply leave the outer wrapper object in place and not change the API layer, so no change. My objective was future-proofing this as much as possible.
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.
this should not be the case, if in the future we'll remove the additionalScopes
in fact we'll need to change the API documentation, and that's should be transparent and described by a new version, because that's a breaking change, otherwise would be a sort of swept under the carpet.
"open" parameters are good when there's no knowledge in the domain about structure and content (see asset properties), but since we know how these additionalScopes
are shaped and we know how to deserialize them, I would describe that clearly in our domain object (CatalogRequest
).
What this PR changes/adds
adds a decision record about adding a new, optional
parameters
field to theCatalogRequest
bodyWhy it does that
documentation
Further notes
List other areas of code that have changed but are not necessarily linked to the main feature. This could be method
signature changes, package declarations, bugs that were encountered and were fixed inline, etc.
Linked Issue(s)
Closes #4322
Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.