-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: adds store_inputs? true/false
option
#29
Conversation
closes: ash-project#20 - introduces `store_inputs?` to the DSL - `store_inputs?` if not included defaults to false - when switched on it ensure an attribute is created to capture the exact inputs of each new version, when the resource we are paper-trailing changes.
@rgraff let me know if this is what you had in mind, look at the test. I think you should be able to tell by that. |
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.
Will let @rgraff take a look a well.
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 this is good and would meet my use case. 🙇♂️
I left some comments and questions.
When we use `store_inputs? true` we store the original inputs of the change as map. Within this map all structs representing Ash Resources are converted into a map using `Ash.Type.dump_to_embedded/3`. We use the function `normalise_inputs/1` to do this which will deeply recurse within maps and lists to ensure all Ash Resource have been converted into their embedded counterpart.
This is a temporary measure, the eventual goal is to inspect each value in the inputs, if that value is sensitive then we'll redact it from the inputs stored in the version
…ords Co-authored-by: Robert Graff <robert@workingdemo.com>
inputs = | ||
changeset.attributes | ||
|> Map.merge(changeset.arguments) | ||
|> normalise_inputs() |
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.
We shouldn't do this by looking for structs, we should get the corresponding argument or attribute and use dump_to_embedded
on it regardless of if the value is a struct or not.
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.
That will take care of the recursive aspect as well.
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.
Just this one open issue. Otherwise, looks good.
Closing for now due to inactivity, happy to revisit at a later point. |
closes: #20
store_inputs?
to the DSLstore_inputs?
if not included defaults to false