Skip to content

Conversation

@anderseknert
Copy link
Member

Fixes #7958

@netlify
Copy link

netlify bot commented Oct 2, 2025

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit 8fe54c8
🔍 Latest deploy log https://app.netlify.com/projects/openpolicyagent/deploys/68de98de3be1d600083c3355
😎 Deploy Preview https://deploy-preview-7959--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@stale
Copy link

stale bot commented Nov 3, 2025

This pull request has been automatically marked as stale because it has not had any activity in the last 30 days.

@stale stale bot added the inactive label Nov 3, 2025
@anderseknert
Copy link
Member Author

tumbleweed-gif-3

@stale stale bot removed the inactive label Nov 7, 2025
Copy link
Member

@sspaink sspaink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!


for _, t := range db.triggers {
t.OnCommit(ctx, txn, event)
if wantsDataConversion && !t.SkipDataConversion {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if wantsDataConversion && !t.SkipDataConversion {
if !t.SkipDataConversion {

Using wantsDataConversion seems unnecessary here, because it will be true if !t.SkipDataConversion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I'll change that, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that turned out to be not true after all, as wantsDataConversion depended on more conditions above.


// Go 48750 ns/op 27040 B/op 311 allocs/op (no additional cost of triggers)
// AST 191501 ns/op 37585 B/op 616 allocs/op (extra cost du to converting back to Go values.. why?)
// AST 191501 ns/op 37585 B/op 616 allocs/op (extra cost du to converting back to Go values)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// AST 191501 ns/op 37585 B/op 616 allocs/op (extra cost du to converting back to Go values)
// AST 191501 ns/op 37585 B/op 616 allocs/op (extra cost due to converting back to Go values)

@netlify
Copy link

netlify bot commented Nov 19, 2025

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit 8c0f368
🔍 Latest deploy log https://app.netlify.com/projects/openpolicyagent/deploys/691daf2df198a8000811997e
😎 Deploy Preview https://deploy-preview-7959--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@anderseknert anderseknert merged commit f2f0ef0 into open-policy-agent:main Nov 19, 2025
31 checks passed
@anderseknert anderseknert deleted the skip-data-conversion branch November 19, 2025 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

inmem: Allow triggers to consume AST data from store

2 participants