-
Notifications
You must be signed in to change notification settings - Fork 684
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
Add missing execution-keyed entities for custom filters #6172
Conversation
Signed-off-by: Katrina Rogan <katroganGH@gmail.com>
Code Review Agent Run #c6ef29Actionable Suggestions - 1
Review Details
|
Changelist by BitoThis pull request implements the following key changes.
|
@@ -138,7 +138,7 @@ func TestListSignals(t *testing.T) { | |||
signalModels := []map[string]interface{}{toSignalMap(*signalModel)} | |||
mockSelectQuery := GlobalMock.NewMock() | |||
mockSelectQuery.WithQuery( | |||
`SELECT * FROM "signals" WHERE project = $1 AND domain = $2 AND name = $3 LIMIT 20`).WithReply(signalModels) | |||
`SELECT * FROM "signals" WHERE execution_project = $1 AND execution_domain = $2 AND execution_name = $3 LIMIT 20`).WithReply(signalModels) |
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.
The SQL query column names have been updated from project
, domain
, name
to execution_project
, execution_domain
, execution_name
. Consider updating the corresponding filter names in the test case to match the new column names for consistency.
Code suggestion
Check the AI-generated fix before applying
- getEqualityFilter(common.Signal, "project", project),
- getEqualityFilter(common.Signal, "domain", domain),
- getEqualityFilter(common.Signal, "name", name),
+ getEqualityFilter(common.Signal, "execution_project", project),
+ getEqualityFilter(common.Signal, "execution_domain", domain),
+ getEqualityFilter(common.Signal, "execution_name", name),
},
Code Review Run #c6ef29
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6172 +/- ##
=======================================
Coverage 37.05% 37.05%
=======================================
Files 1318 1318
Lines 132606 132606
=======================================
Hits 49136 49136
Misses 79220 79220
Partials 4250 4250
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Why are the changes needed?
Bug introduced in #5935
Entities in this list have execution-key as part of their primary key. Execution key columns are all prefixed with "execution_" under the hood, so we need to prepend the column values with this for DB queries to work.
What changes were proposed in this pull request?
See PR title
How was this patch tested?
Deployed to my sandbox and ran the
waiting_for_external_inputs
workflow on sandbox.Then ran this script
and ensured it succeeded:
Labels
Please add one or more of the following labels to categorize your PR:
This is important to improve the readability of release notes.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This PR addresses a bug in execution-keyed entities handling by adding NodeExecutionEvent and Signal to the execution identifier entities map. The SQL query column names in signal repository tests have been updated with 'execution_' prefix for proper database querying. These changes ensure correct handling of execution-keyed entities in database operations.Unit tests added: True
Estimated effort to review (1-5, lower is better): 1