-
-
Notifications
You must be signed in to change notification settings - Fork 60
feat(cbrs): Snuba Admin UI (webpage) #7399
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
Conversation
aa7f5f4 to
ff2a13a
Compare
|
✅ All tests passed |
| runMigration: (req: RunMigrationRequest) => Promise<RunMigrationResult>; | ||
| getAllowedTools: () => Promise<AllowedTools>; | ||
| getStoragesWithAllocationPolicies: () => Promise<string[]>; | ||
| getRoutingStrategies: () => Promise<string[]>, |
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.
| getRoutingStrategies: () => Promise<string[]>, | ||
| getAllocationPolicies: (storage: string) => Promise<AllocationPolicy[]>; | ||
| setAllocationPolicyConfig: ( | ||
| getRoutingStrategyConfigs: (strategy_name: string) => Promise<StrategyData>; |
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.
This endpoint retrieves all of the strategy configurations as well as the allocation policies, so I don't know if getRoutingStrategyConfigs is the best name - I'm open to other suggestions
| getAllocationPolicies: (storage: string) => Promise<AllocationPolicy[]>; | ||
| setAllocationPolicyConfig: ( | ||
| getRoutingStrategyConfigs: (strategy_name: string) => Promise<StrategyData>; | ||
| setConfigurableComponentConfiguration: ( |
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.
The content of setAllocationPolicyConfig and deleteAllocationPolicyConfig is no longer specific to allocation policy anymore so I changed their names to setConfigurableComponentConfiguration and deleteConfigurableComponentConfiguration
| }; | ||
|
|
||
| type AllocationPolicyConfig = { | ||
| type Configuration = { |
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.
This isn't really specific to AllocationPolicy anymore - a configuration is a configuration, so I changed the names in this file accordingly
| edit: ReactNode; | ||
| }; | ||
|
|
||
| type ConfigurableComponentData = { |
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.
These mirror the ConfigurableComponentData, PolicyData, and StrategyData in the backend
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.
I decided to keep this file in /capacity_management because allocation policies are originally a CapMan concpet
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 don't need this anymore: #7346 (comment) 🦅 🦅 🦅
35a9f43 to
c303581
Compare
| deleteOrReset().toLowerCase() + | ||
| " this config?" |
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.
auto-reformatting
| export function PolicyRenderer({ api, policies, resourceIdentifier, resourceType }: PolicyRendererProps) { | ||
| function renderPolicies(policies: AllocationPolicy[]) { | ||
| if (!resourceIdentifier) { | ||
| return <p>{resourceType} not selected.</p>; |
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.
I'm also down to have this as return <p>Resource not selected.</p>; so we don't have to have an extra parameter resourceType
| }, | ||
| name: "key1", | ||
| value: "10", | ||
| description: "something", |
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.
auto-reformat
| resourceType: string; | ||
| } | ||
|
|
||
| const policyTypeStyle = { |
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.
I'm going to change this to ConfigurableComponentTypeStyle
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.
yes please
| renderContent: (data: T | undefined, selectedResource: string | undefined) => React.ReactNode; | ||
| } | ||
|
|
||
| function ConfigurableComponentRenderer<T>({ |
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.
this is more of a dropdown renderer, it has nothing to do with configurable component
3d9e20a to
5f780f0
Compare
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.
I tried to make a new file after renaming but git still recognize the renaming instead
volokluev
left a comment
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.
Looks good overall, have some comments you can resolve in this PR or a different one
| resourceType: string; | ||
| } | ||
|
|
||
| const policyTypeStyle = { |
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.
yes please
| if (configurableComponentData.configurable_component_namespace === "BaseRoutingStrategy") { | ||
| return COLORS.SNUBA_BLUE; | ||
| } |
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.
I think you could probably rewrite this logic to be more generic. I.e. if there is no is_active or is_enforced config, the table color is blue. I don't think you need to have this conditional specific to the routing strategies in this generic component
| description="Placeholder for now", | ||
| value_type=int, | ||
| default=50, | ||
| param_types={"organization_id": int}, |
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.
Not for this PR but a follow up here would be to put the configurations that exist in this strategy (i.e. everywhere that we use get_config in this class, to use the configuration instead

Uh oh!
There was an error while loading. Please reload this page.