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.

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

Client key for read operations #462

Merged
merged 5 commits into from
Jul 7, 2023

Conversation

smallhive
Copy link
Contributor

closes #429

@smallhive
Copy link
Contributor Author

Many commits, to be able to make changes in each place separately. Before merging, we may squash them for few

@smallhive smallhive marked this pull request as ready for review July 5, 2023 12:18
Copy link
Contributor

@cthulhu-rider cthulhu-rider left a comment

Choose a reason for hiding this comment

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

overall LGTM. Lets sync minor documentation moments

client/container.go Outdated Show resolved Hide resolved
client/object_delete.go Outdated Show resolved Hide resolved
client/container.go Outdated Show resolved Hide resolved
@smallhive smallhive force-pushed the 429-client-key-for-read-operations branch from 453521e to d4dec97 Compare July 5, 2023 13:06
@smallhive smallhive force-pushed the 429-client-key-for-read-operations branch from d4dec97 to bb13b99 Compare July 6, 2023 04:40
Copy link
Member

@carpawell carpawell left a comment

Choose a reason for hiding this comment

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

  1. Conflicts.
  2. There is a commit about the tests, does that mean that before that commit the tests did not pass? I would not go that way: I think after every commit build and tests should be OK.
  3. The last commit message does not end with a dot and also it seems more like "Created/Is created" rather than "Creates".

@smallhive smallhive force-pushed the 429-client-key-for-read-operations branch from bb13b99 to d5e1aef Compare July 6, 2023 12:15
@smallhive
Copy link
Contributor Author

  1. Conflicts.
  2. There is a commit about the tests, does that mean that before that commit the tests did not pass? I would not go that way: I think after every commit build and tests should be OK.
  3. The last commit message does not end with a dot and also it seems more like "Created/Is created" rather than "Creates".
  1. Fixed
  2. I agree. Meanwhile, these tests are marked by a special build tag and don't affect test pipelines. But ok, divided commit and moved to another
  3. What do you mean? I don't use dots in commits ends. And didn't catch an idea about meaning 😃

@smallhive smallhive requested a review from carpawell July 6, 2023 12:19
@cthulhu-rider
Copy link
Contributor

I think after every commit build and tests should be OK

this makes sense, but sometimes you just add unit test which shows some bug (so test fails), and then add fix commit (which in particular fixes test) so u can feel the impact of new changes. Otherwise, test should always pass, yeah

@cthulhu-rider
Copy link
Contributor

cthulhu-rider commented Jul 6, 2023

@smallhive i guess last commit's message looks a bit wierd and out-of-context, we could clearly write
Touched methods never return ErrMissingSigner because signer is initialized in the constructor, so nil-check is always passed.

but i don't insist, me personally can realize any form

@smallhive
Copy link
Contributor Author

@smallhive i guess last commit's message looks a bit wierd and out-of-context, we could clearly write Touched methods never return ErrMissingSigner because signer is initialized in the constructor, so nil-check is always passed.

but i don't insist, me personally can realize any form

I just wanted to left some context in extended commit message

@cthulhu-rider
Copy link
Contributor

I just wanted to left some context in extended commit message

and this is great and must have, but iiuc @carpawell was confused by grammar

@carpawell
Copy link
Member

carpawell commented Jul 6, 2023

Meanwhile, these tests are marked by a special build tag and don't affect test pipelines.

but sometimes you just add unit test which shows some bug (so test fails), and then add fix commit (which in particular fixes test) so u can feel the impact of new changes.

Well, I would not go that way anyway. As it is described in our git page with rules, "This is important to have an ability to bisect problematic changes if there is a need to."

I don't use dots in commits ends.

Oh, well, I was not attentive enough then. Hm, it is not declared explicitly but we have an example there:

subsystem: short description (up to 80 symbols, better up to 60)

Long description with references, links, test data and whatever else is needed to explain what, why and how is changed.

Signed-off-by: Your Name email@example.com

I would interpret that as a rule that says that any sentence should end with a period (if it is not a header). Anyway, I would say that all the commits we have done until that PR end with dots if they contain a body.

  1. [...] And didn't catch an idea about meaning

@smallhive, I meant that I did not get the idea of the "Creates in the constructor". Who creates? What are that words about?

For Read operation Default signer exists by default in client.

Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
@smallhive smallhive force-pushed the 429-client-key-for-read-operations branch from d5e1aef to 6bfae0f Compare July 7, 2023 03:54
Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
Instead of SetDefaultSigner, we have ephemeral key, which we generate in the constructor. In other words we have default signer by default.
Removed tests are not actual, because these methods can't return ErrMissingSigner any more.

closes #429

Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
These methods never return ErrMissingSigner because the signer was created in the constructor and available all time.

Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
@smallhive smallhive force-pushed the 429-client-key-for-read-operations branch from 6bfae0f to a51cf37 Compare July 7, 2023 04:11
@smallhive
Copy link
Contributor Author

@smallhive, I meant that I did not get the idea of the "Creates in the constructor". Who creates? What are that words about?

I got it, I updated the description messages in the commits

@cthulhu-rider cthulhu-rider merged commit e6dd19a into master Jul 7, 2023
@cthulhu-rider cthulhu-rider deleted the 429-client-key-for-read-operations branch July 7, 2023 17:07
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.

Client key for read operations
3 participants