feat: Add toggle for automatic allocation creation in panel settings #1884
Conversation
- Introduced `create_new` configuration to control automatic allocation behavior. - Updated allocation service logic to respect the `create_new` toggle. - Enhanced panel settings UI to include the new `create_new` toggle option. - Added helper text for better clarity on configuration options.
📝 WalkthroughWalkthroughAdds a runtime feature flag and admin toggle to control whether the allocation service may create new allocations when no unassigned allocation exists; the config, translations, Filament settings UI, and allocation service behavior are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Service as FindAssignableAllocationService
participant Repo as AllocationRepo
participant Config
Caller->>Service: findAssignableAllocation()
Service->>Repo: query for unassigned allocation
Repo-->>Service: allocation or null
alt Unassigned allocation exists
Service-->>Caller: return allocation
else No unassigned allocation
Service->>Config: read client_features.allocations.create_new
Config-->>Service: boolean
alt create_new = true
rect rgb(200,220,255)
Note over Service,Repo: create allocation within configured port range
Service->>Repo: create allocation
Repo-->>Service: new allocation
Service-->>Caller: return allocation
end
else create_new = false
rect rgb(255,220,200)
Note over Service: throw NoAutoAllocationSpaceAvailableException
Service-->>Caller: throw exception
end
end
end
Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Services/Allocations/FindAssignableAllocationService.php (1)
56-67: Consider restructuring for clarity.The logic is functionally correct, but the control flow could be more explicit. The comment on line 61 states "If create_new is enabled, create a new allocation if none available," but line 62 also handles the case where an allocation already exists (via the null coalescing operator).
Consider this more explicit structure:
- // If create_new is disabled, only pick from existing allocations - if (!$createNew && !$allocation) { - throw new NoAutoAllocationSpaceAvailableException(); - } - - // If create_new is enabled, create a new allocation if none available - $allocation = $allocation ?? $this->createNewAllocation($server); + if (!$allocation) { + // If create_new is disabled, only pick from existing allocations + if (!$createNew) { + throw new NoAutoAllocationSpaceAvailableException(); + } + + // Create a new allocation if create_new is enabled + $allocation = $this->createNewAllocation($server); + }This makes it clearer that:
- We only enter the conditional block when no allocation exists
- We either throw an exception or create a new allocation based on the setting
- If an allocation already exists, we skip the entire block
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/Filament/Admin/Pages/Settings.php(1 hunks)app/Services/Allocations/FindAssignableAllocationService.php(3 hunks)config/panel.php(1 hunks)lang/en/admin/setting.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T17:33:57.388Z
Learnt from: lancepioch
Repo: pelican-dev/panel PR: 1595
File: app/Services/Servers/ServerCreationService.php:182-184
Timestamp: 2025-08-12T17:33:57.388Z
Learning: In the Pelican Panel codebase, when updating allocations to assign them to servers, the preferred pattern is to use whereNull('server_id'), lockForUpdate(), and direct property assignment with save() rather than mass update methods, to prevent race conditions and mass-assignment issues while ensuring model events fire properly.
Applied to files:
app/Services/Allocations/FindAssignableAllocationService.php
🧬 Code graph analysis (1)
app/Services/Allocations/FindAssignableAllocationService.php (1)
app/Models/Server.php (1)
allocation(277-280)
🔇 Additional comments (4)
config/panel.php (1)
36-36: LGTM!The new configuration option is properly structured, follows the existing pattern, and defaults to
trueto maintain backward compatibility with current behavior.app/Filament/Admin/Pages/Settings.php (1)
625-636: LGTM!The new toggle is well-integrated and follows existing patterns in the settings UI. The visibility constraint (only shown when allocations are enabled) is appropriate, and all UI elements (label, helper text, icons, colors, state cast) are properly configured.
app/Services/Allocations/FindAssignableAllocationService.php (2)
24-35: LGTM!The updated docblock clearly explains the new behavior: attempting to find existing allocations first, then either creating new ones (if enabled) or throwing an exception (if disabled).
42-42: LGTM!The configuration read properly defaults to
true, maintaining backward compatibility with existing behavior.
|
Doesn't seem worth adding, if an allocation is created by an admin it will be locked by default already. |
|
But if I provide a large port range and want to exclude certain ports from being assigned, this comes in very handy because I can create the assignable ports beforehand and then use this option. see moreOr, in my use case, I have two nodes and can have different port ranges for each one by creating the ports beforehand and then giving a range that includes all and using this option to prevent ports I don't want from being used.I know it's a small change and can be seen as not needed, but I would benefit greatly from this, and I think others can appreciate it as well. |
…-allocation-creation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/Services/Allocations/FindAssignableAllocationService.php (1)
52-59: Consider using the preferred allocation assignment pattern (pre-existing).Based on learnings from the codebase, the preferred pattern for assigning allocations to servers includes using
lockForUpdate()when querying and direct property assignment instead of mass updates. This prevents race conditions and ensures model events fire properly.While this is pre-existing code (not introduced by this PR), consider refactoring to:
$allocation = Allocation::withoutGlobalScopes() ->where('node_id', $server->node_id) ->when($server->allocation, function ($query) use ($server) { $query->where('ip', $server->allocation->ip); }) ->whereNull('server_id') ->lockForUpdate() // Add lock ->inRandomOrder() ->first(); // ... (conditional logic) ... // Replace line 69 $allocation->server_id = $server->id; $allocation->save();Based on learnings
Also applies to: 69-69
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Filament/Admin/Pages/Settings.php(1 hunks)app/Services/Allocations/FindAssignableAllocationService.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Filament/Admin/Pages/Settings.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T17:33:57.388Z
Learnt from: lancepioch
Repo: pelican-dev/panel PR: 1595
File: app/Services/Servers/ServerCreationService.php:182-184
Timestamp: 2025-08-12T17:33:57.388Z
Learning: In the Pelican Panel codebase, when updating allocations to assign them to servers, the preferred pattern is to use whereNull('server_id'), lockForUpdate(), and direct property assignment with save() rather than mass update methods, to prevent race conditions and mass-assignment issues while ensuring model events fire properly.
Applied to files:
app/Services/Allocations/FindAssignableAllocationService.php
🧬 Code graph analysis (1)
app/Services/Allocations/FindAssignableAllocationService.php (1)
app/Models/Server.php (1)
allocation(277-280)
🔇 Additional comments (4)
app/Services/Allocations/FindAssignableAllocationService.php (4)
24-28: LGTM! Clear documentation of the new behavior.The updated docblock accurately describes the new
create_newtoggle behavior and clarifies the control flow.
61-64: LGTM! Logic correctly implements the toggle behavior.The conditional correctly prevents automatic allocation creation when the toggle is disabled and no unassigned allocations are available.
45-46: LGTM! Comments accurately reflect the new behavior.The updated inline comments clearly document the conditional logic based on the
create_newtoggle.Also applies to: 66-66
42-42: Config key verified and properly defined.The config key
panel.client_features.allocations.create_newis correctly defined inconfig/panel.php:36with the default valuetrue, matching the implementation. No issues found.
|
@PalmarHealer Please resolve conflicts. |
…-allocation-creation
3 participants
create_newconfiguration to control automatic allocation behavior.create_newtoggle.create_newtoggle option.Supersedes #1852