-
Notifications
You must be signed in to change notification settings - Fork 199
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
Policy Store: Add PolicyEntity and PolicyTypes #1133
base: main
Are you sure you want to change the base?
Conversation
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. Left minor comments! Thanks @HonahX for the PR.
|
||
@JsonIgnore | ||
public int getPolicyTypeCode() { | ||
String policyTypeCode = getPropertiesAsMap().get(POLICY_TYPE_CODE_KEY); |
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.
We will need policy type to be a top-level field instead of hidden in property map in the future, but it's fine now.
import jakarta.annotation.Nullable; | ||
|
||
/* Represents all predefined policy types in Polaris */ | ||
public enum PredefinedPolicyType implements PolicyType { |
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.
Minor: a name like SystemPolicyTypes
? Just wanna brainstorming a bit.
} | ||
|
||
public static class Builder extends PolarisEntity.BaseBuilder<PolicyEntity, Builder> { | ||
public Builder(Namespace namespace, String policyName) { |
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.
Per design, policy type is a required field, should we put it in the constructor as well?
As discussed in the sync meeting, if possible, please hold this PR until a NoSQL peristence impl. is available (hopefully soon). |
Hi @dimas-b, do you mean hold #1104 which includes the new Persistence Table, interface and impl? For that, I marked #1104 as draft to allow some time for ongoing refactoring and new impl. For this one, the PolicyEntity will be persisted just like other existing entities. So I think it should not be related to any changes in persistence refactoring or new persistence impl and we can proceed this in parallel with ongoing persistence layer work. Will NoSQL persistence impl introduce any changes that affect the current persistence model for Entitites? |
Current NoSQL POC using typeCode (#1112), so adding new types will cause some interference, hence my request to hold this PR as well. |
I think it’s unreasonable to hold multiple PRs due to one persistence implementation. The NoSQL implementation isn’t the only ongoing persistence change—@singhpk234 and I are actively working on the Postgres implementation as well. Given that multiple efforts are running in parallel, we shouldn’t allow one refactor to block overall development. |
@flyrain I agee. However, I'm a bit surprised @singhpk234 and you are working on PostgreSQL backend while the "new" API are not yet finalize. @dennishuo and @dimas-b are still working on it. I just want to avoid dual effort. |
This PR introduces basic classes for supporting Policy Store in Polaris, as discussed in #1059.
This will be the first step to unblock all other Policy PRs and discussions
cc: @flyrain