TrueCloudLab
/
policy-engine
generated from TrueCloudLab/basic
18
0
Fork 8
Code Issues 2 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

chain: Refactor ObjectType type #75

Merged
fyrchik merged 2 commits from aarifullin/policy-engine:fix/refactor_cnr_obj_type into master 2024-05-13 15:30:32 +00:00
Conversation 0 Commits 2 Files Changed 13
aarifullin commented 2024-05-07 14:35:53 +00:00
Collaborator

Probably, ObjectType was mistaken for frostfs object types, but actually policy-engine knows (or should know) nothing about container and objects (the exception is schema constants).

  • Rename ObjectType to Kind;
  • Rename Object field in Condition to ConditionKind;
  • Regenerate easy-json marshalers/unmarshalers;
  • Remove ContainerResource, ContainerRequest constants as they
    should not be used at all;
  • Fix unit-tests.
Probably, `ObjectType` was mistaken for frostfs object types, but actually policy-engine knows (or should know) nothing about container and objects (the exception is schema constants). * Rename `ObjectType` to `Kind`; * Rename `Object` field in `Condition` to `ConditionKind`; * Regenerate easy-json marshalers/unmarshalers; * Remove `ContainerResource`, `ContainerRequest` constants as they should not be used at all; * Fix unit-tests.
aarifullin added 1 commit 2024-05-07 14:35:55 +00:00
DCO action / DCO (pull_request) Failing after 1m5s Details
Tests and linters / Tests (1.21) (pull_request) Successful in 1m7s Details
Tests and linters / Tests (1.20) (pull_request) Successful in 1m23s Details
Tests and linters / Staticcheck (pull_request) Successful in 1m31s Details
Tests and linters / Tests with -race (pull_request) Successful in 1m43s Details
Tests and linters / Lint (pull_request) Successful in 2m19s Details
8459fe38d1 [#XX] chain: Factor out condition object types for container 
Signed-off-by: Airat Arifullin <aarifullin@yadro.com>
aarifullin requested review from acid-ant 2024-05-07 14:36:07 +00:00
aarifullin requested review from storage-core-committers 2024-05-07 14:36:07 +00:00
aarifullin requested review from storage-core-developers 2024-05-07 14:36:09 +00:00
fyrchik reviewed 2024-05-07 15:16:22 +00:00
pkg/chain/chain.go Outdated
@ -71,1 +70,4 @@
Op ConditionType
// Object specifies for which data this condition will be applied to (Resource, Request).
// Object should be not confused with the same name entity from FrostFS.
Object ObjectType
fyrchik commented 2024-05-07 15:16:22 +00:00
Owner

It definitely causes confusion. What about renaming it to Kind?
The type should also be named Kind and constants KindResource, KindRequest.

It definitely causes confusion. What about renaming it to `Kind`? The type should also be named `Kind` and constants `KindResource`, `KindRequest`.
aarifullin commented 2024-05-07 16:31:14 +00:00
Poster
Collaborator

Yeah, Kind really sounds good 👍

Yeah, `Kind` really sounds good 👍
aarifullin commented 2024-05-07 16:34:28 +00:00
Poster
Collaborator

But this breaks backward compatibility - "old" chains with Object won't be parsed

But this breaks backward compatibility - "old" chains with `Object` won't be parsed
fyrchik commented 2024-05-08 07:28:24 +00:00
Owner

Why, though? The binary format is unchanged, besides removed items, which is unrelated to naming.

Btw if removing these constant breaks backward-compatibility with v0.38.4 version of node, we shouldn't do it.

Why, though? The binary format is unchanged, besides removed items, which is unrelated to naming. Btw if removing these constant breaks backward-compatibility with v0.38.4 version of node, we shouldn't do it.
fyrchik commented 2024-05-08 07:30:50 +00:00
Owner

Breaking compatibility in frostfs-cli is fine, though.

Breaking compatibility in frostfs-cli is fine, though.
aarifullin force-pushed fix/refactor_cnr_obj_type from 8459fe38d1 to f859839a7d 2024-05-08 07:25:06 +00:00 Compare
dstepanov-yadro approved these changes 2024-05-08 08:24:59 +00:00
dstepanov-yadro approved these changes 2024-05-13 08:03:10 +00:00
acid-ant approved these changes 2024-05-13 08:44:08 +00:00
aarifullin force-pushed fix/refactor_cnr_obj_type from f859839a7d to 7b876453ac 2024-05-13 10:11:11 +00:00 Compare
aarifullin changed title from chain: Factor out condition object types for container to chain: Refactor ObjectType type 2024-05-13 10:11:25 +00:00
aarifullin force-pushed fix/refactor_cnr_obj_type from 7b876453ac to dbf41eb77f 2024-05-13 10:13:13 +00:00 Compare
aarifullin requested review from dstepanov-yadro 2024-05-13 10:15:52 +00:00
aarifullin requested review from acid-ant 2024-05-13 10:15:53 +00:00
aarifullin requested review from storage-services-committers 2024-05-13 10:16:07 +00:00
aarifullin requested review from storage-services-developers 2024-05-13 10:16:07 +00:00
aarifullin force-pushed fix/refactor_cnr_obj_type from dbf41eb77f to 43f1ac9851 2024-05-13 10:55:31 +00:00 Compare
fyrchik reviewed 2024-05-13 10:59:47 +00:00
pkg/chain/chain.go Outdated
@ -73,2 +71,2 @@
Key string
Value string
Op ConditionType
ConditionKind ConditionKindType
fyrchik commented 2024-05-13 10:59:46 +00:00
Owner

It is ok for a type name, but why do we use Condition prefix in the field name? It leads to Condition.ConditionKind full name and some verbosity in JSON format.
Also, Kind is somewhat synonymous with Type.

So what about Kind ConditionKind?

It is ok for a type name, but why do we use `Condition` prefix in the field name? It leads to `Condition.ConditionKind` full name and some verbosity in JSON format. Also, `Kind` is somewhat synonymous with `Type`. So what about `Kind ConditionKind`?
aarifullin commented 2024-05-13 14:33:55 +00:00
Poster
Collaborator

Your point is absolutely fair but the point is that we've got similar names throuought: db36131800/policy/policy_contract.go (L14) (this is explanation for the rest of reviewers who didn't get the point of this renaming yet)

But I agreed that Condition.ConditionKind really looks ugly. Let's try the solution suggested by you 'Kind ConditionKind`

Your point is absolutely fair but the point is that we've got similar names throuought: https://git.frostfs.info/TrueCloudLab/frostfs-contract/src/commit/db361318009cc9c9df97133c8c2acedad4eb2fd4/policy/policy_contract.go#L14 (this is explanation for the rest of reviewers who didn't get the point of this renaming yet) But I agreed that `Condition.ConditionKind` really looks ugly. Let's try the solution suggested by you 'Kind ConditionKind`
aarifullin commented 2024-05-13 14:37:15 +00:00
Poster
Collaborator

UPD: Renamed to Kind ConditionKind

UPD: Renamed to `Kind ConditionKind`
aarifullin force-pushed fix/refactor_cnr_obj_type from 43f1ac9851 to 84c4872b20 2024-05-13 14:36:34 +00:00 Compare
fyrchik merged commit 84c4872b20 into master 2024-05-13 15:30:32 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
dstepanov-yadro
acid-ant
TrueCloudLab/storage-core-developers
TrueCloudLab/storage-services-committers
TrueCloudLab/storage-services-developers
Labels
Clear labels
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff   
blocked

The issue or PR is blocked by something   
bug

Something is not working   
config

Configuration format update or breaking change   
discussion

Talking about something in order to reach a decision or to exchange ideas   
documentation

Documentation related   
duplicate

This issue or pull request already exists   
enhancement

New feature   
go

Language features adoption   
help wanted

Extra attention is needed   
internal
   
invalid

Something is wrong   
kludge

This is a kludge and we know it   
observability

Related to metrics, tracing and logs   
perfomance

Perfomance related   
question

More information is needed   
refactoring

Refactoring   
wontfix

This won't be fixed
No Label
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
4 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/policy-engine#75
There is no content yet.
Delete Branch "aarifullin/policy-engine:fix/refactor_cnr_obj_type"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?