feat: add tagged to Journaled::Change - #82
Conversation
|
|
||
| module Journaled | ||
| VERSION = "6.2.9" | ||
| VERSION = "6.3.0" |
There was a problem hiding this comment.
This is a breaking change for anyone already consuming the event, so we'll want to go to 7.0.0 here.
There was a problem hiding this comment.
(specifically, the change to journaled_schemas/journaled/change.json means that consumers will need to handle tags even if it's empty / not present by default)
There was a problem hiding this comment.
Updated to 7!
| it 'includes :tags only in the tagged instance\'s payload' do | ||
| tagged_change | ||
|
|
||
| expect(untagged_change.journaled_attributes.keys).not_to include(:tags) |
There was a problem hiding this comment.
👍 this is the desired behavior that gives consumers a migration path.
smudge
left a comment
smudge
left a comment
There was a problem hiding this comment.
happy to debate whether or not this is truly merits a major version bump, but looking at this from the perspective of consumers, the gem now allows for a local implementation change that will pass CI checks but then potentially break consumers in production, so I was thinking that we could use the major release to flag that.
Alternatively, we would need some way of detecting and failing if someone adds tagged: true to an existing directive (forcing them to acknowledge that consumers are prepared for this new field), but I wasn't seeing a clear path to incorporating such a check into the consuming app's own runtime behavior (such that the consuming app would have a chance to have its own tests fail).
| This is useful for capturing metadata (like an impersonating actor) that isn't | ||
| itself a column on the model. It defaults to `false` to preserve the existing | ||
| event shape for models that don't opt in. |
There was a problem hiding this comment.
| This is useful for capturing metadata (like an impersonating actor) that isn't | |
| itself a column on the model. It defaults to `false` to preserve the existing | |
| event shape for models that don't opt in. | |
| This is useful for capturing metadata (like a session or visitor ID) that isn't | |
| itself a column on the model. It defaults to `false` to preserve the existing | |
| event shape for models that don't opt in. |
Nitpick here, but I think transient IDs are a more common use case for README purposes.
There was a problem hiding this comment.
I made this change in the PR description but forgot it here, good catch!
smudge
left a comment
smudge
left a comment
There was a problem hiding this comment.
apologies for the slow review -- and my slowness resulting in this needing a rebase 😞
I added one nitpick (take it or leave it, but I figure since you're rebasing anyways)
The rest looks great -- thank you for this contribution! 🙌
Add support for the gem's existing "tagged" mechanism to `Journaled::Change`, so apps that use `Journaled.tag!`/`Journaled.tagged` to attach per-request context (e.g. actor metadata, request ID, span, job ID, etc) can now see that context on change events.
96203f3 to
bd01068
Compare
Invalidated by push of bd01068
2 participants
Add support for the gem's existing "tagged" mechanism to
Journaled::Change, so apps that useJournaled.tag!/Journaled.taggedto attach per-request context (e.g. actor metadata, request ID, span, job ID, etc) can now see that context on change events.