ape: Make RemoveChainLocalOverride set correct removed flag #1006

Merged
fyrchik merged 1 commit from aarifullin/frostfs-node:fix/898_removed-true into master 2024-02-28 19:07:10 +00:00
Member
  • If LocalStorage error status is NotFound, then set removed flag to true anyway.

Close #898

* If LocalStorage error status is NotFound, then set removed flag to true anyway. Close #898
aarifullin requested review from storage-core-committers 2024-02-26 08:58:34 +00:00
aarifullin requested review from storage-core-developers 2024-02-26 08:58:44 +00:00
acid-ant approved these changes 2024-02-26 09:23:18 +00:00
fyrchik reviewed 2024-02-26 12:17:09 +00:00
@ -164,3 +164,3 @@
code := getCodeByLocalStorageErr(err)
if code == codes.NotFound {
removed = false
removed = true
Owner

So what do we need remove flag for if it is always true?

So what do we need `remove` flag for if it is always true?
Author
Member

You're right. I have removed remove flag. But removed in the response body is left as a first implementation flavor, because we agreed about to return success even a chain is not found after control API had introduced Remove method

You're right. I have removed `remove` flag. But `removed` in the response body is left as a first implementation *flavor*, because we agreed about to return *success* even a chain is not found after control API had introduced `Remove` method
Owner

I don't understand, what is the reason to have field in the body which is always true?
Is someone already dependent on it?

I don't understand, what is the reason to have field in the body which is always true? Is someone already dependent on it?
aarifullin force-pushed fix/898_removed-true from 7d3bed0821 to 63a8ef63d4 2024-02-26 12:45:14 +00:00 Compare
fyrchik reviewed 2024-02-26 12:59:14 +00:00
@ -171,3 +168,3 @@
resp := &control.RemoveChainLocalOverrideResponse{
Body: &control.RemoveChainLocalOverrideResponse_Body{
Removed: removed,
Removed: true,
Owner

Do we need this in control service at all?

Do we need this in control service at all?
Author
Member

Alright. It seems Removed is really confusing flag. I have removed it proto, regenerated protobufs and fix control API and cli!

Alright. It seems `Removed` is really confusing flag. I have removed it `proto`, regenerated protobufs and fix control API and cli!
aarifullin force-pushed fix/898_removed-true from 63a8ef63d4 to b19b268baa 2024-02-27 09:29:47 +00:00 Compare
aarifullin force-pushed fix/898_removed-true from b19b268baa to 470862baab 2024-02-27 09:30:06 +00:00 Compare
aarifullin requested review from acid-ant 2024-02-27 09:31:26 +00:00
acid-ant approved these changes 2024-02-27 10:15:55 +00:00
fyrchik approved these changes 2024-02-28 19:07:00 +00:00
fyrchik merged commit 93bf9acbc2 into master 2024-02-28 19:07:10 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-developers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#1006
No description provided.