Skip to content

Propogate SQLTransactionTimeout#5345

Merged
MarcosDY merged 3 commits intospiffe:mainfrom
faisal-memon:5343
Aug 4, 2024
Merged

Propogate SQLTransactionTimeout#5345
MarcosDY merged 3 commits intospiffe:mainfrom
faisal-memon:5343

Conversation

@faisal-memon
Copy link
Copy Markdown
Contributor

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

Affected functionality
Events based cache

Description of change
Fixes bug where SQLTransactionTimeout is not propagated

Which issue this PR fixes

fixes #5343

Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Comment thread pkg/server/server.go
CacheReloadInterval: s.config.CacheReloadInterval,
EventsBasedCache: s.config.EventsBasedCache,
PruneEventsOlderThan: s.config.PruneEventsOlderThan,
SQLTransactionTimeout: s.config.SQLTransactionTimeout,
Copy link
Copy Markdown
Member

@amoore877 amoore877 Jul 31, 2024

Choose a reason for hiding this comment

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

it'd be nice to unit test, considering lack thereof caused the gap

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hey @amoore877 That was my first thought too. Unfortunately I dont know where to work in a unit test here as the fields just end up as unexported fields in the endpoints object.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah, and there's no way to easily test with a running module and infer behavior from config I guess. sad times.

the most I've done sometimes in such cases at least verify those unexported fields (which would be accessible in a same-package test) are set as expected. it's one of those kinds of tests that seems silly at first, but catches simple mistakes.

better though would be that behavioral check, but that seems hard compared to the effort of the fix :)

@MarcosDY MarcosDY added this to the 1.10.2 milestone Aug 3, 2024
@MarcosDY MarcosDY merged commit cdf8645 into spiffe:main Aug 4, 2024
@faisal-memon faisal-memon deleted the 5343 branch February 14, 2025 18:02
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.

[Event-based entry cache] New server config for sql_transaction_timeout is not propagated to endpoints config

3 participants