TrueCloudLab/policy-engine
21
0
Fork 11
generated from TrueCloudLab/basic
Code Issues 5 Pull requests 2 Projects Releases Packages Wiki Activity Actions

chain: Refactor ObjectType type #75

Merged
fyrchik merged 2 commits from aarifullin/policy-engine:fix/refactor_cnr_obj_type into master 2024-09-04 19:51:23 +00:00
Conversation 0 Commits 2 Files changed 13 +283 -285
aarifullin commented 2024-05-07 14:35:53 +00:00
Member
Copy link

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 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
Copy link

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
Author
Member
Copy link

Yeah, Kind really sounds good 👍

Yeah, `Kind` really sounds good 👍
aarifullin commented 2024-05-07 16:34:28 +00:00
Author
Member
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Author
Member
Copy link

Your point is absolutely fair but the point is that we've got similar names throuought:

Line 14 in TrueCloudLab/frostfs-contract@db36131
type Kind byte

(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
Author
Member
Copy link

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 labels
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 items
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
No description provided.
Delete branch "aarifullin/policy-engine:fix/refactor_cnr_obj_type"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?