-
Notifications
You must be signed in to change notification settings - Fork 22
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
feat: Add retry logic for bulk imports based on work in the zed CLI #165
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
} | ||
) | ||
|
||
type RetryableClient struct { |
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.
Add doc comments on all exported functions and types
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.
I believe I have done this now. If I missed one, let me know.
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.
missed one:
// NewRetryableClient initializes a brand new client for interacting
// with Authzed.
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.
Done ✅
I have read the CLA Document and I hereby sign the CLA |
Signed-off-by: Chance Murray <[email protected]>
bf1b18c
to
6fb5141
Compare
Signed-off-by: Chance Murray <[email protected]>
v1/retryable_client.go
Outdated
) | ||
|
||
// ConflictStrategy is an enumeration type that represents the strategy to be used | ||
// when a conflict occurs during a bulk import of relationships in Authzed. |
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.
into SpiceDB
type ConflictStrategy int | ||
|
||
const ( | ||
Fail ConflictStrategy = iota |
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.
Move the comment for each to above each value
v1/retryable_client.go
Outdated
} | ||
) | ||
|
||
// RetryableClient represents an open connection to Authzed with |
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.
SpiceDB
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.
I'll fix this, but I was following the pattern set on the doc comments for the other clients. They all say Authzed.
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.
Yeah, we're transitioning those to make it match the fact that this is open source
Signed-off-by: Chance Murray <[email protected]>
d4811e0
to
6e04b69
Compare
Signed-off-by: Chance Murray <[email protected]>
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.
LGTM
Tests can be added later. I just wanted an initial review of the code first.