Skip to content

fix: Remove MaxCreatedTime from DomainAuditLogFilter#7725

Merged
natemort merged 1 commit intocadence-workflow:masterfrom
joannalauu:fix/remove-maxcreatedtime
Feb 20, 2026
Merged

fix: Remove MaxCreatedTime from DomainAuditLogFilter#7725
natemort merged 1 commit intocadence-workflow:masterfrom
joannalauu:fix/remove-maxcreatedtime

Conversation

@joannalauu
Copy link
Contributor

@joannalauu joannalauu commented Feb 19, 2026

What changed?

  • Removed MaxCreatedTime from DomainAuditLogFilter

Why?

  • Remove duplicate logic between MaxCreatedTime and PageMaxCreatedTime in domain audit filtering

Part of: #7602

How did you test it?

  • Unit tests: go test -v ./common/persistence/sql -run TestGetDomainAuditLogs TestSelectFromDomainAuditLogs

Potential risks
N/A, logic stays the same

Release notes
N/A

Documentation Changes
N/A


Reviewer Validation

PR Description Quality (check these before reviewing code):

  • "What changed" provides a clear 1-2 line summary
    • Project Issue is linked
  • "Why" explains the full motivation with sufficient context
  • Testing is documented:
    • Unit test commands are included (with exact go test invocation)
    • Integration test setup/commands included (if integration tests were run)
    • Canary testing details included (if canary was mentioned)
  • Potential risks section is thoughtfully filled out (or legitimately N/A)
  • Release notes included if this completes a user-facing feature
  • Documentation needs are addressed (or noted if uncertain)

Signed-off-by: Joanna Lau <118241363+joannalauu@users.noreply.github.com>
@joannalauu joannalauu changed the title Remove MaxCreatedTime from DomainAuditLogFilter fix: Remove MaxCreatedTime from DomainAuditLogFilter Feb 19, 2026
@gitar-bot
Copy link

gitar-bot bot commented Feb 19, 2026

🔍 CI failure analysis for ad76d58: The codecov CI job is failing due to a pre-existing test failure in TestWatchLoopDisabled, which is unrelated to this PR's changes to the domain audit filtering layer.

Issue: Codecov Test Failure (Unrelated to PR Changes)

The codecov CI job is failing due to a test failure in:

github.com/uber/cadence/service/sharddistributor/client/spectatorclient

Root Cause

The test TestWatchLoopDisabled failed at clientimpl_test.go:285:

Error: An error is expected but got nil.

This test expects an error to be returned but receives nil instead, indicating a test assertion problem.

Assessment: Unrelated to This PR

This PR modifies files in the domain audit filtering area:

  • common/persistence/sql/sql_domain_audit_store.go
  • common/persistence/sql/sql_domain_audit_store_test.go
  • common/persistence/sql/sqlplugin/interfaces.go
  • common/persistence/sql/sqlplugin/postgres/domain_audit_log.go
  • common/persistence/sql/sqlplugin/postgres/domain_audit_log_test.go

The failing test is in the sharddistributor/client/spectatorclient package, which:

  1. Has no dependency relationship with the domain audit persistence layer
  2. Is in a completely different service component (shard distribution vs. persistence)
  3. Has been failing consistently across multiple CI runs, suggesting a pre-existing issue

Details

This failure has persisted across multiple CI runs for this PR, confirming it is not a flaky test but rather a pre-existing broken test in the codebase that predates these changes.

Code Review ✅ Approved

Clean removal of redundant MaxCreatedTime filter field. The PageMaxCreatedTime field already provides the same upper bound constraint, and all SQL queries, parameter bindings, and tests are consistently updated.

Rules ❌ No requirements met

Repository Rules

PR Description Quality Standards: Expand Why section to explain how MaxCreatedTime and PageMaxCreatedTime both worked previously, and fix test command syntax to use proper regex alternation: go test -v ./common/persistence/sql -run 'TestGetDomainAuditLogs|TestSelectFromDomainAuditLogs'

1 rule not applicable. Show all rules by commenting gitar display:verbose.

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@Shaddoll
Copy link
Member

lgtm, could you include the unit test command?

@joannalauu
Copy link
Contributor Author

lgtm, could you include the unit test command?

Done. Do you know why the codecov test is failing? I don't believe my changes affect that test.

@fimanishi
Copy link
Member

lgtm, could you include the unit test command?

Done. Do you know why the codecov test is failing? I don't believe my changes affect that test.

It's a flaky test. it's being addressed in #7728

@natemort natemort merged commit 1a42a94 into cadence-workflow:master Feb 20, 2026
50 of 53 checks passed
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.

5 participants