Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add caveat support to postgres datastore #890

Merged
merged 3 commits into from
Oct 14, 2022
Merged

add caveat support to postgres datastore #890

merged 3 commits into from
Oct 14, 2022

Conversation

vroldanbet
Copy link
Contributor

@vroldanbet vroldanbet commented Oct 7, 2022 •
edited
Loading

Tracking: #386
Closes #884
Some improvements in the context of #753

What

introduces caveat support in postgres datastore

How

  • refactors memdb caveat test and turns it into a datastore test. Datastores not implementing the interface are skipped
  • adds migration to add caveat table for PSQL, and contextualized caveat to relational_tuple table
  • makes psql TX implements CaveatStorer, following same strategy for MVCC
  • adds context to ReadCaveatByName to follow same signature as ReadNamespace
  • adjusts memdb to avoid upserts on duplicate caveats, and align with PSQL
  • adds missing test exercising Watch API with caveated relationships
  • adds missing test exercising DeleteCaveats and implements method in memdb

@vroldanbet vroldanbet self-assigned this Oct 7, 2022
@github-actions github-actions bot added area/api v1 Affects the v1 API area/CLI Affects the command line area/datastore Affects the storage system area/dispatch Affects dispatching of requests labels Oct 7, 2022
@jzelinskie jzelinskie added the area/caveats Affects caveated relationships label Oct 8, 2022
@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Oct 10, 2022
@vroldanbet vroldanbet force-pushed the add-caveats-to-postgres branch from 5fa2dde to 350d39f Compare October 10, 2022 11:50
@github-actions github-actions bot added area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) and removed area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Oct 10, 2022
@vroldanbet vroldanbet force-pushed the add-caveats-to-postgres branch 3 times, most recently from 27e69e7 to ba96442 Compare October 10, 2022 16:50
@github-actions github-actions bot removed the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Oct 10, 2022
@vroldanbet vroldanbet force-pushed the add-caveats-to-postgres branch 3 times, most recently from c21dfad to 5d340e6 Compare October 10, 2022 18:14
@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Oct 10, 2022
@vroldanbet vroldanbet marked this pull request as ready for review October 10, 2022 18:55
@vroldanbet vroldanbet requested a review from a team October 10, 2022 18:55
vroldanbet
vroldanbet commented Oct 10, 2022
View reviewed changes
internal/datastore/postgres/common/pgx.go Show resolved Hide resolved
internal/datastore/postgres/migrations/driver.go Show resolved Hide resolved
internal/datastore/postgres/migrations/zz_migration.0013_caveat.go Outdated Show resolved Hide resolved
internal/datastore/postgres/postgres_test.go Outdated Show resolved Hide resolved
pkg/cmd/migrate.go Show resolved Hide resolved
pkg/migrate/migrate.go Show resolved Hide resolved
@jakedt jakedt self-requested a review October 11, 2022 15:14
jakedt
jakedt previously requested changes Oct 11, 2022
View reviewed changes
internal/datastore/postgres/postgres_test.go Outdated Show resolved Hide resolved
@vroldanbet vroldanbet force-pushed the add-caveats-to-postgres branch from 8afc26c to 0dd9aba Compare October 13, 2022 08:50
@github-actions github-actions bot removed the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Oct 13, 2022
@vroldanbet vroldanbet force-pushed the add-caveats-to-postgres branch from 0dd9aba to b2cd09a Compare October 13, 2022 10:30
josephschorr
josephschorr reviewed Oct 13, 2022
View reviewed changes
internal/datastore/common/sql.go Outdated Show resolved Hide resolved
internal/datastore/memdb/caveat.go Outdated Show resolved Hide resolved
internal/datastore/postgres/common/pgx.go Show resolved Hide resolved
internal/datastore/postgres/readwrite.go Outdated Show resolved Hide resolved
pkg/datastore/test/caveat.go Show resolved Hide resolved
@vroldanbet vroldanbet force-pushed the add-caveats-to-postgres branch from b2cd09a to 6f6071d Compare October 13, 2022 17:20
@vroldanbet vroldanbet force-pushed the add-caveats-to-postgres branch 2 times, most recently from 9e965df to 58e4a2f Compare October 13, 2022 17:35
josephschorr
josephschorr previously approved these changes Oct 13, 2022
View reviewed changes
Copy link
Member

@josephschorr josephschorr left a comment

Choose a reason for hiding this comment

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

LGTM from my side

@vroldanbet vroldanbet mentioned this pull request Oct 13, 2022
Merged
8 tasks
@vroldanbet
improve logging for spicedb migrate cmd
aa3712f 
- errors are returned instead of calling Fatal()
- improve logging messages
- remove redundant Alembic driver message
- added log when already running at target revision
@vroldanbet
adds caveat support to postgres
cb7b956 
- refactors memdb caveat test and turns it into
  a datastore test. Datastores not implementing
  the interface are skipped
- adds migration to add caveat table for PSQL
- makes psql TX implements CaveatStorer, following
  same strategy for MVCC
- adds context to ReadCaveatByName to follow same
  signature as ReadNamespace
- adjusts memdb to avoid upserts on duplicate caveats,
  and align with PSQL
- implement missing DeleteCaveat test, and implement
  that logic for memdb and postgres
@vroldanbet vroldanbet force-pushed the add-caveats-to-postgres branch from 58e4a2f to cb7b956 Compare October 14, 2022 13:18
@vroldanbet vroldanbet enabled auto-merge October 14, 2022 13:18
@vroldanbet vroldanbet removed the request for review from jakedt October 14, 2022 13:22
@vroldanbet vroldanbet dismissed jakedt’s stale review October 14, 2022 13:23

Jake is happy with one single approval in this PR

@vroldanbet vroldanbet force-pushed the add-caveats-to-postgres branch from a281e65 to 39da0e0 Compare October 14, 2022 15:21
@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Oct 14, 2022
@vroldanbet
add timeout to datastore docker boot
255853d 
Sometimes CI times out after 30 minutes,
and the stack trace shows it was blocked
waiting for a connection

this would allow it to have a chance to retry,
or at least fail fast
@vroldanbet vroldanbet force-pushed the add-caveats-to-postgres branch from 39da0e0 to 255853d Compare October 14, 2022 15:52
@github-actions github-actions bot removed the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Oct 14, 2022
josephschorr
josephschorr approved these changes Oct 14, 2022
View reviewed changes
Copy link
Member

@josephschorr josephschorr left a comment

Choose a reason for hiding this comment

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

LGTM

@vroldanbet vroldanbet merged commit c445f19 into main Oct 14, 2022
@vroldanbet vroldanbet deleted the add-caveats-to-postgres branch October 14, 2022 16:11
@github-actions github-actions bot locked and limited conversation to collaborators Oct 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@josephschorr josephschorr josephschorr approved these changes

@jakedt jakedt jakedt left review comments

Assignees

@vroldanbet vroldanbet

Labels
area/api v1 Affects the v1 API area/caveats Affects caveated relationships area/CLI Affects the command line area/datastore Affects the storage system area/dispatch Affects dispatching of requests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

support caveats in postgres datastore
4 participants
@vroldanbet @jakedt @josephschorr @jzelinskie