-
Notifications
You must be signed in to change notification settings - Fork 14
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/replicate with signature #622
Conversation
87e4722
to
9e5750d
Compare
API version is 01229b49e9165e693a48e2b96ccad528df34b55e. Signed-off-by: Pavel Karpy <[email protected]>
Signed-off-by: Pavel Karpy <[email protected]>
gRPC conflicts if more than one generated package is `init`ed, therefore, it either local packages or remove neofs-api-go should be used. Signed-off-by: Pavel Karpy <[email protected]>
9e5750d
to
d2fcec1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #622 +/- ##
==========================================
- Coverage 53.93% 53.93% -0.01%
==========================================
Files 164 164
Lines 19222 19249 +27
==========================================
+ Hits 10368 10381 +13
- Misses 8415 8426 +11
- Partials 439 442 +3 ☔ View full report in Codecov by Sentry. |
f17c599
to
75f6e88
Compare
|
||
return msg, nil | ||
} | ||
|
||
type replicateResponse struct { | ||
err error | ||
_sigRequested bool |
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.
C-style?
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.
Looks weird. And it's not a response field, really.
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.
this is the only possibility of understanding a signature is missing but was requested in the current code. we also may just do nothing if it is missing and return nil, it is just an RPC wrapper without any logic then, maybe that is good too, not sure
And it's not a response field, really
yes, that is why it is underscored...
Looks weird
... which is available by the language and not prohibited by any of our linters
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 overall
1st commit with codegen for proto
is inaffectual here, right?
@@ -48,38 +55,38 @@ import ( | |||
// replicated object; | |||
// - [apistatus.ErrContainerNotFound]: the container to which the replicated | |||
// object is associated was not found. | |||
func (c *Client) ReplicateObject(ctx context.Context, id oid.ID, src io.ReadSeeker, signer neofscrypto.Signer) error { | |||
func (c *Client) ReplicateObject(ctx context.Context, id oid.ID, src io.ReadSeeker, signer neofscrypto.Signer, signedReplication bool) (*neofscrypto.Signature, error) { |
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.
in which cases signedReplication
gonna be false in practice?
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.
For non-initial replication?
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.
also as i understand, it will stiil be possible to skip this for some sort of container after nspcc-dev/neofs-api#300, no @roman-khimov?
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.
Yes, containers with different consistency policies can have different requirements wrt this as well.
Signed-off-by: Pavel Karpy <[email protected]>
Should have been part of ce6f86d, but IDE fooled me and did not search test files. Signed-off-by: Pavel Karpy <[email protected]>
75f6e88
to
b322ea8
Compare
Yes, i had plans for it but they failed, so let it just be a commit that keeps things in sync. |
@@ -95,7 +95,7 @@ func RecordN(n int) *eacl.Record { | |||
|
|||
func TableN(n int) *eacl.Table { | |||
rs := make([]eacl.Record, n) | |||
for i := 0; i < n; i++ { | |||
for i := range n { |
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.
Not really related.
No description provided.