feat: Add support for custom actions on apps #340
Conversation
Enable defining and running custom actions beyond standard lifecycle hooks. Users can now trigger these actions via the new API endpoint and CLI command. Blueprint files can define named actions that can be executed on demand.
WalkthroughThis update introduces support for running arbitrary custom actions on applications via both API and CLI. It adds new YAML blueprint actions, extends the internal action name representation, implements a new API endpoint and handler for custom actions, and updates the CLI to allow users to trigger these actions. Supporting error handling, state machine execution, and sequence management are also included. Additionally, UI components were added to display and trigger custom actions in the frontend. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant API
participant AppRegistry
participant DockerEngine
User->>CLI: Run `scottyctl app:action <app> <action>`
CLI->>API: POST /api/v1/apps/{app}/actions { action_name }
API->>AppRegistry: Check if app exists
AppRegistry-->>API: App info / error
API->>DockerEngine: Run custom action on app (state machine)
DockerEngine-->>API: Task started (RunningAppContext)
API-->>CLI: Respond with RunningAppContext
CLI->>API: Poll for task completion
API-->>CLI: Task finished, updated app info
CLI-->>User: Display success and app info
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
config/blueprints/drupal-lagoon.yaml (1)
23-28: LGTM! Consider command reuse to reduce duplication.The new named actions provide excellent flexibility for on-demand execution of drush commands. However, there's duplication with existing
post_runandpost_rebuildactions.Consider defining shared command sequences to reduce duplication:
+ shared_commands: + drush_uli_cmd: + - drush uli --uri=$SCOTTY__PUBLIC_URL__NGINX + drush_deploy_cmd: + - drush deploy --uri=$SCOTTY__PUBLIC_URL__NGINX actions: post_rebuild: cli: - - drush deploy --uri=$SCOTTY__PUBLIC_URL__NGINX + - !include shared_commands.drush_deploy_cmd post_run: cli: - - drush uli --uri=$SCOTTY__PUBLIC_URL__NGINX + - !include shared_commands.drush_uli_cmd drush:uli: cli: - - drush uli --uri=$SCOTTY__PUBLIC_URL__NGINX + - !include shared_commands.drush_uli_cmd drush:deploy: cli: - - drush deploy --uri=$SCOTTY__PUBLIC_URL__NGINX + - !include shared_commands.drush_deploy_cmdscotty/src/docker/run_app_custom_action.rs (1)
119-131: Consider adding action parameter to logging context.The function has proper instrumentation but could benefit from including the action name in the tracing context for better observability.
-#[instrument(skip(app_state))] +#[instrument(skip(app_state), fields(action = ?action, app_name = %app.name))] pub async fn run_app_custom_action( app_state: SharedAppState, app: &AppData, action: ActionName, ) -> anyhow::Result<RunningAppContext> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
config/blueprints/drupal-lagoon.yaml(1 hunks)config/blueprints/nginx-lagoon.yaml(1 hunks)scotty-core/src/apps/shared_app_list.rs(1 hunks)scotty-core/src/settings/app_blueprint.rs(1 hunks)scotty/src/api/error.rs(1 hunks)scotty/src/api/handlers/apps/custom_action.rs(1 hunks)scotty/src/api/handlers/apps/mod.rs(1 hunks)scotty/src/api/router.rs(4 hunks)scotty/src/docker/mod.rs(1 hunks)scotty/src/docker/run_app_custom_action.rs(1 hunks)scottyctl/src/cli.rs(2 hunks)scottyctl/src/commands/apps.rs(2 hunks)scottyctl/src/main.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
scotty/src/docker/mod.rs (1)
scotty/src/docker/run_app_custom_action.rs (1)
run_app_custom_action(120-131)
scotty/src/api/router.rs (1)
scotty/src/api/handlers/apps/custom_action.rs (1)
run_custom_action_handler(45-66)
🔇 Additional comments (23)
scotty/src/api/handlers/apps/mod.rs (1)
2-2: LGTM! Clean module addition.The addition of the
custom_actionmodule follows the existing pattern and integrates well with the overall custom action feature.scotty/src/docker/mod.rs (1)
10-10: LGTM! Well-positioned module addition.The
run_app_custom_actionmodule is properly placed in alphabetical order and follows the naming conventions of other modules in this file.config/blueprints/nginx-lagoon.yaml (1)
23-25: Great example of a custom action in blueprint configuration.The new
statusaction demonstrates how custom actions can be defined in blueprints. Thenginx -vcommand is appropriate for checking nginx version/status.scotty-core/src/apps/shared_app_list.rs (1)
52-54: LGTM! Clean and efficient app existence check.The
has_appmethod is well-implemented with appropriate use of a read lock for the read-only operation. The method signature and logic are consistent with the existing codebase patterns.scottyctl/src/main.rs (1)
49-49: LGTM! Consistent command handling implementation.The new
Commands::Actionvariant follows the established pattern of other command handlers, with proper async execution and error propagation.scottyctl/src/cli.rs (2)
63-65: LGTM! Well-structured CLI command definition.The
app:actioncommand follows the established naming convention and is properly documented.
124-131: LGTM! Clean command structure.The
ActionCommandstruct has the appropriate fields needed for custom action execution. The field names are clear and the documentation is helpful.scotty/src/api/router.rs (3)
31-31: LGTM! Proper imports for the new handler.The imports for both the OpenAPI path macro and the handler function are correctly added.
Also applies to: 60-60
98-98: LGTM! Handler properly included in OpenAPI documentation.The
run_custom_action_handleris correctly added to the OpenAPI paths list for API documentation.
164-167:✅ Verification successful
LGTM! Well-structured API route definition.
The new route follows established patterns:
- Uses POST method which is appropriate for action execution
- Path parameter
{app_name}matches the handler's expected parameter- Route is properly secured within the protected router
- Consistent formatting with other route definitions
Verify that the handler integration works correctly by checking the parameter mapping:
🏁 Script executed:
#!/bin/bash # Description: Verify the handler parameter mapping matches the route definition # Expected: Handler should accept Path(app_name) parameter matching the route {app_name} # Check the handler function signature ast-grep --pattern 'pub async fn run_custom_action_handler($$$) { $$$ }'Length of output: 76
🏁 Script executed:
#!/bin/bash # Locate the definition of run_custom_action_handler and inspect its signature rg -n "run_custom_action_handler" -A3 -B2 .Length of output: 2284
Handler signature correctly matches route parameter
Verified that
run_custom_action_handlerinscotty/src/api/handlers/apps/custom_action.rsaccepts
Path(app_name): Path<String>, matching the{app_name}route parameter. All looks good.
- Uses POST method which is appropriate for action execution
- Path parameter
{app_name}confirmed in handler signature- Route is properly secured within the protected router
- Consistent formatting with other route definitions
scottyctl/src/commands/apps.rs (2)
8-9: LGTM!Import addition is appropriate for the new custom action functionality.
204-240: Excellent implementation following established patterns.The
run_custom_actionfunction is well-structured and follows the same patterns as other app command functions:
- Proper UI status updates
- Consistent error handling with context
- Appropriate use of existing utility functions (
get_or_post,wait_for_task,get_app_info)- Consistent success messaging and output formatting
scotty-core/src/settings/app_blueprint.rs (4)
5-5: LGTM!Removing automatic serde derives is necessary for the custom serialization implementation.
11-12: Excellent extension of the ActionName enum.The
Custom(String)variant enables support for arbitrary user-defined action names while maintaining the existing predefined actions.
14-26: Well-implemented manual serialization.The serialization logic correctly:
- Maps predefined variants to their snake_case string representations
- Maps the
Customvariant directly to its inner string valueThis approach maintains backward compatibility while enabling flexible custom action names.
28-57: Proper deserialization implementation.The custom visitor and deserialize implementation correctly:
- Maps known action strings to their corresponding enum variants
- Falls back to
Custom(String)for unknown action names- Follows standard serde visitor patterns
scotty/src/api/handlers/apps/custom_action.rs (3)
20-28: Well-defined request and response schemas.The payload structs are properly structured with appropriate serde and OpenAPI schema derives for API documentation.
30-44: Comprehensive OpenAPI documentation.The API endpoint is thoroughly documented with:
- All relevant HTTP status codes (200, 401, 404, 500)
- Proper request/response schema references
- Security requirements (bearerAuth)
- Clear path and operation description
45-66: Excellent API handler implementation.The handler demonstrates good practices:
- Proper logging of the operation start
- App existence validation with descriptive error handling
- Clean integration with the custom action execution logic
- Appropriate use of the new
ActionName::Customvariant- Consistent error handling patterns
scotty/src/docker/run_app_custom_action.rs (4)
1-26: LGTM! Well-organized imports and dependencies.The imports are logically organized and all necessary dependencies are properly included for the state machine implementation.
27-34: LGTM! Clear state machine progression.The enum defines a logical progression of states for custom action execution, following the expected lifecycle from Docker login through completion.
42-77: Robust validation logic with good error handling.The validation sequence properly checks all prerequisites before action execution. The nested unwraps are safe due to the preceding null checks.
125-127: LGTM! Proper legacy app handling.The check prevents custom actions from running on unsupported legacy apps, which is the correct behavior.
Enhance blueprint functionality with: - New blueprint:info CLI command for detailed blueprint information - Action descriptions field for documenting purpose of each action - Improved status action in nginx-lagoon blueprint with better output - Color formatting in blueprint info display for better readability
Add dedicated Action struct with descriptions and commands, removing the separate action_descriptions map. Update all blueprints to use the new format and modify code to handle the restructured data.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
scottyctl/src/commands/blueprints.rs (1)
56-66: Consider using pattern matching for cleaner Option handling.The current pattern of checking
is_none()followed byunwrap()works but could be more idiomatic.Apply this diff to use pattern matching:
-// Find the specified blueprint -let blueprint = blueprints.blueprints.get(&cmd.blueprint_name); -if blueprint.is_none() { - return Err(anyhow::anyhow!( - "Blueprint {} not found", - cmd.blueprint_name.yellow() - )); -} - -let blueprint = blueprint.unwrap(); +// Find the specified blueprint +let blueprint = blueprints.blueprints.get(&cmd.blueprint_name) + .ok_or_else(|| anyhow::anyhow!( + "Blueprint {} not found", + cmd.blueprint_name.yellow() + ))?;scotty-core/src/settings/app_blueprint.rs (1)
81-82: Remove unnecessary#[allow(unused)]attribute.The
AppBlueprintstruct is actively used throughout the codebase, so this attribute appears to be unnecessary.#[derive(Debug, Clone, Serialize, Deserialize, utoipa::ToSchema, utoipa::ToResponse)] -#[allow(unused)] pub struct AppBlueprint {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
config/blueprints/drupal-lagoon.yaml(1 hunks)config/blueprints/nginx-lagoon.yaml(1 hunks)scotty-core/src/notification_types/mod.rs(3 hunks)scotty-core/src/settings/app_blueprint.rs(3 hunks)scotty/src/api/error.rs(2 hunks)scotty/src/docker/run_app_custom_action.rs(1 hunks)scotty/src/docker/state_machine_handlers/run_post_actions_handler.rs(1 hunks)scotty/src/settings/config.rs(1 hunks)scotty/tests/test_docker_registry_password.yaml(1 hunks)scottyctl/src/cli.rs(4 hunks)scottyctl/src/commands/blueprints.rs(2 hunks)scottyctl/src/main.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- scotty/tests/test_docker_registry_password.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- scottyctl/src/main.rs
- config/blueprints/nginx-lagoon.yaml
- config/blueprints/drupal-lagoon.yaml
- scotty/src/api/error.rs
- scotty/src/docker/run_app_custom_action.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
scottyctl/src/commands/blueprints.rs (3)
scottyctl/src/api.rs (1)
get(153-155)scottyctl/src/context.rs (2)
ui(29-31)new(21-26)scottyctl/src/utils/ui.rs (1)
new(28-57)
🔇 Additional comments (19)
scotty/src/docker/state_machine_handlers/run_post_actions_handler.rs (1)
71-72: LGTM! Action structure refactoring looks good.The change correctly adapts to the new
Actionstruct abstraction where commands are accessed via thecommandsfield. This aligns well with the broader refactoring to support custom actions and provides better encapsulation of action data.scotty/src/settings/config.rs (1)
188-188: Good refactoring to use dedicated method.Using
get_commands_for_servicemethod instead of manual nested map access improves code maintainability and provides better abstraction over the internal action structure. This makes the test code more robust to future changes in the action representation.scotty-core/src/notification_types/mod.rs (3)
7-7: Import looks good.The
ActionNameimport is correctly added to support the newAppCustomActionCompletedmessage variant.
44-44: Good addition of custom action completion notification.The new
AppCustomActionCompleted(ActionName)variant properly extends the notification system to support custom action events, following the established pattern for other app lifecycle events.
56-59: Clear and informative message formatting.The message format clearly communicates which custom action was executed and on which app. Using the Debug format for
action_nameprovides good visibility into the action details.scottyctl/src/cli.rs (4)
63-65: Good addition of custom action CLI command.The
app:actioncommand follows the established naming pattern and provides a clear interface for users to execute custom actions on apps.
79-81: Useful blueprint information command.The
blueprint:infocommand is a valuable addition that will help users understand available blueprints and their capabilities before using them.
100-104: Clear command structure definition.The
BlueprintInfoCommandstruct properly defines the required parameter with clear documentation.
134-141: Well-defined action command structure.The
ActionCommandstruct clearly defines the required parameters for executing custom actions, with appropriate documentation for both the app name and action name arguments.scottyctl/src/commands/blueprints.rs (4)
1-1: LGTM! Imports are appropriate for the new functionality.The added imports support color formatting and the new blueprint info command effectively.
Also applies to: 7-8
34-35: LGTM! Table width adjustments improve readability.The column width changes provide better space allocation for description and services columns.
89-172: Well-structured action display with clear visual distinction.The separation of lifecycle and custom actions with different formatting (dimmed vs green) provides excellent visual clarity. The service command formatting with indentation and bullet points enhances readability.
181-186: Helpful user guidance for custom actions.The note about executing custom actions provides valuable context for users discovering available actions.
scotty-core/src/settings/app_blueprint.rs (6)
5-12: LGTM! Flexible enum design for custom actions.The addition of the
Custom(String)variant enables arbitrary custom action names while maintaining backward compatibility with predefined lifecycle actions.
14-57: Well-implemented custom serialization for ActionName.The manual Serialize/Deserialize implementation correctly handles both predefined and custom action names, ensuring consistent string representation in configuration files.
61-79: Clean Action struct design with useful helper methods.The Action struct effectively encapsulates action metadata with description and service-specific commands. The helper methods provide convenient access patterns.
110-128: Well-designed helper methods for action access.The helper methods provide a clean API for accessing actions and their commands, with proper Option handling for safety.
130-165: Comprehensive validation logic.The validate method thoroughly checks that all referenced services (in public services and actions) are included in the required services list, preventing configuration errors.
108-108: Good practice implementing std::error::Error trait.Implementing the Error trait for AppBlueprintValidationError enables better error handling integration with the ? operator and error propagation.
Implement a dropdown menu that allows users to trigger custom actions defined in app blueprints. Integrate this component into the app dashboard page and redirect users to the task view when an action is triggered.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
frontend/src/components/custom-actions-dropdown.svelte (2)
18-33: Potential improvement: Add error boundary for blueprint fetching.The blueprint fetching logic is well-structured, but consider adding more specific error handling for different failure scenarios (network issues, authorization, invalid blueprint format).
- } else { - console.warn( - `Blueprint ${app.settings.app_blueprint} not found or has no actions` - ); - } + } else { + console.warn( + `Blueprint ${app.settings.app_blueprint} not found or has no actions` + ); + customActions = []; // Ensure empty array for consistent state + }
63-65: Consider logging less sensitive information.Avoid logging the full API response which might contain sensitive information. Log only necessary debugging details.
- } else { - console.error('Unexpected API response:', result); - throw new Error('Invalid response from server'); - } + } else { + console.error('Invalid API response: missing task ID'); + throw new Error('Invalid response from server'); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/custom-actions-dropdown.svelte(1 hunks)frontend/src/routes/dashboard/[slug]/+page.svelte(2 hunks)frontend/src/routes/tasks/[slug]/+page.svelte(0 hunks)scotty-core/src/settings/app_blueprint.rs(3 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/routes/tasks/[slug]/+page.svelte
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Suite
- GitHub Check: Test Suite
🔇 Additional comments (9)
frontend/src/routes/dashboard/[slug]/+page.svelte (2)
17-17: Clean import of the new custom actions component.The import is properly placed and follows the existing pattern.
95-112: Well-structured layout update for custom actions integration.The flex layout with proper spacing and conditional rendering is implemented correctly. The divider provides good visual separation between standard and custom actions, and the conditional rendering ensures the dropdown only appears when the app is in a supported state.
frontend/src/components/custom-actions-dropdown.svelte (2)
73-75: Well-designed state validation function.The
isSupportedfunction correctly checks both app status and blueprint presence, ensuring custom actions are only available when appropriate.
100-111: Excellent accessibility and UX implementation.The dropdown implementation includes proper tooltips, disabled states, and loading indicators, providing a good user experience.
scotty-core/src/settings/app_blueprint.rs (5)
11-11: Good extension of ActionName enum for custom actions.The addition of
Custom(String)variant properly extends the enum to support arbitrary action names while maintaining backward compatibility with predefined actions.
61-79: Well-designed Action struct with proper encapsulation.The new
Actionstruct clearly separates description from commands and provides good helper methods. Theget_commands_for_servicemethod properly encapsulates the HashMap access.
108-108: Good addition of standard Error trait implementation.Implementing
std::error::ErrorforAppBlueprintValidationErrorfollows Rust best practices and improves error handling throughout the application.
147-159: Comprehensive validation logic for action dependencies.The validation correctly ensures that all services referenced in actions are included in the required services list. This prevents runtime errors when actions reference undefined services.
14-26:✅ Verification successful
Verify serialization format consistency.
The manual
Serializeimplementation looks correct, but ensure this format is consistent with existing YAML blueprints and API expectations.
🏁 Script executed:
#!/bin/bash # Description: Check existing blueprint YAML files to verify action naming format # Expected: Find how actions are currently named in YAML files echo "Checking blueprint YAML files for action naming patterns..." fd -e yaml -e yml . config/ | xargs grep -A 5 -B 5 "actions:" || echo "No blueprint files found" fd -e yaml -e yml . | xargs grep -A 5 -B 5 "post_" || echo "No post_ actions found"Length of output: 7248
Serialization format consistency confirmed
The
ActionNameSerializeimpl uses"post_create","post_run", and"post_rebuild", which exactly matches the keys found underactions:inconfig/blueprints/*.yaml. No further changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scottyctl/src/commands/blueprints.rs (1)
53-62: Consider API efficiency for single blueprint lookup.The current implementation fetches all blueprints and then searches for the specific one. While simple, this approach may not scale well with many blueprints. Consider if the API supports fetching a single blueprint by name.
If the backend supports it, consider adding an endpoint like
GET /blueprints/{name}to fetch individual blueprints more efficiently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scotty-core/src/settings/app_blueprint.rs(3 hunks)scottyctl/src/commands/blueprints.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scotty-core/src/settings/app_blueprint.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Suite
- GitHub Check: Check
- GitHub Check: Test Suite
🔇 Additional comments (4)
scottyctl/src/commands/blueprints.rs (4)
1-8: LGTM: Import additions support the new functionality well.The new imports for
OwoColorize,BlueprintInfoCommand, andActionNameare appropriate for the enhanced blueprint information display with colored output and custom action support.
34-35: LGTM: Column width adjustments improve display.The increased column widths (25 and 40 characters) will better accommodate longer descriptions and service lists in the blueprint table.
43-193: LGTM: Comprehensive blueprint information display.The
blueprint_infofunction provides excellent user experience with:
- Clear error handling for non-existent blueprints
- Well-formatted tables for public services and actions
- Color-coded differentiation between lifecycle and custom actions
- Helpful usage notes for running custom actions
- Proper integration with the custom actions feature
The implementation correctly handles the new
ActionName::Customvariant and displays comprehensive action information.
179-183: LGTM: Helpful usage guidance for users.The note about how to execute custom actions provides excellent user guidance and completes the user journey from discovering available actions to actually running them.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
scottyctl/src/commands/blueprints.rs (4)
133-133: Fix typo in comment.There's a typo in the comment: "wabacon" should be "way".
- // Format the services and commands in a readable wabacon + // Format the services and commands in a readable way
60-187: Consider breaking down this large function for better maintainability.The
blueprint_infofunction is quite large (127 lines) and handles multiple responsibilities:
- API data fetching
- Blueprint lookup and validation
- Basic info formatting
- Public services table creation
- Actions table creation with lifecycle/custom action handling
- Output formatting
While the logic is correct, this could be improved by extracting some functionality into smaller helper functions.
Consider extracting the following into separate functions:
format_blueprint_basic_info- lines 82-90format_public_services_table- lines 92-101build_actions_table- lines 107-170This would improve readability and make the function easier to test and maintain.
121-125: Consider using a more maintainable approach for action string mapping.The hardcoded match statement for converting
ActionNameto string could be replaced with a method on the enum or a more systematic approach.Consider adding a
to_stringoras_strmethod to theActionNameenum in the core library, or use a helper function to centralize this mapping:- let action_str = match action { - ActionName::PostCreate => "post_create", - ActionName::PostRun => "post_run", - ActionName::PostRebuild => "post_rebuild", - _ => continue, - }; + let action_str = action.as_lifecycle_str().unwrap_or_else(|| continue);
141-142: Consider using constants for action type labels.The hardcoded strings "Lifecycle" and "Custom" could be moved to constants for better maintainability.
+const ACTION_TYPE_LIFECYCLE: &str = "Lifecycle"; +const ACTION_TYPE_CUSTOM: &str = "Custom"; ... - "Lifecycle", + ACTION_TYPE_LIFECYCLE, ... - "Custom", + ACTION_TYPE_CUSTOM,Also applies to: 156-157
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scottyctl/src/commands/blueprints.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Suite
- GitHub Check: Test Suite
🔇 Additional comments (1)
scottyctl/src/commands/blueprints.rs (1)
45-58: LGTM: Helper function addresses previous duplication concern.The
format_services_commandhelper function successfully addresses the previous review comment about duplicated formatting logic. The implementation correctly formats the HashMap of services and commands with proper color styling and indentation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
scottyctl/src/commands/blueprints.rs (1)
45-58: Excellent refactoring! Addresses the duplication concern.This helper function successfully eliminates the code duplication that was flagged in previous reviews. The implementation correctly formats services and commands with consistent styling.
scotty-core/src/settings/app_blueprint.rs (3)
26-33: Breaking change: Ensure proper migration strategy.The addition of
Custom(String)variant toActionNameis a breaking change that affects existing blueprint files and dependent code.Please verify that migration documentation and version handling are in place:
#!/bin/bash # Description: Check for migration documentation and version handling # Expected: Find migration guides, version checks, or compatibility handling # Check for changelog or migration documentation fd -e md -e txt | xargs rg -l "migration|breaking|version" # Check for version handling in blueprint loading rg -A 5 -B 5 "version|migrate|compat" --type rust
66-75: 🛠️ Refactor suggestionAdd input validation for custom action names.
The deserialization logic accepts any string as a custom action name without validation, which could lead to security or compatibility issues.
impl From<String> for ActionName { fn from(value: String) -> Self { match value.as_str() { "post_create" => ActionName::PostCreate, "post_run" => ActionName::PostRun, "post_rebuild" => ActionName::PostRebuild, - custom => ActionName::Custom(custom.to_string()), + custom => { + if custom.chars().all(|c| c.is_alphanumeric() || c == '_' || c == '-') + && !custom.is_empty() + && !custom.starts_with('-') { + ActionName::Custom(custom.to_string()) + } else { + // For now, fall back to the original behavior + // In a future version, this should return an error + ActionName::Custom(custom.to_string()) + } + } } } }Consider implementing proper error handling in the
Deserializeimplementation:impl<'de> Deserialize<'de> for ActionName { fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> where D: serde::Deserializer<'de>, { + use serde::de::Error; let s = String::deserialize(deserializer)?; - Ok(s.into()) + match s.as_str() { + "post_create" => Ok(ActionName::PostCreate), + "post_run" => Ok(ActionName::PostRun), + "post_rebuild" => Ok(ActionName::PostRebuild), + custom => { + if custom.chars().all(|c| c.is_alphanumeric() || c == '_' || c == '-') + && !custom.is_empty() + && !custom.starts_with('-') { + Ok(ActionName::Custom(custom.to_string())) + } else { + Err(D::Error::custom(format!("Invalid action name format: {}", custom))) + } + } + } } }
110-117: 🛠️ Refactor suggestionCritical: Document the breaking change in the actions field.
The change from
HashMap<ActionName, HashMap<String, Vec<String>>>toHashMap<ActionName, Action>needs proper documentation.#[derive(Debug, Clone, Serialize, Deserialize, utoipa::ToSchema, utoipa::ToResponse)] pub struct AppBlueprint { pub name: String, pub description: String, + /// Actions available for this blueprint. Each action contains a description + /// and a mapping of service names to command lists. + /// + /// **Breaking Change**: Previously this was a nested HashMap structure. + /// See migration guide for updating existing blueprint files. pub actions: HashMap<ActionName, Action>, pub required_services: Vec<String>, pub public_services: Option<HashMap<String, u16>>, }
🧹 Nitpick comments (3)
scottyctl/src/commands/blueprints.rs (3)
133-133: Fix the typo in the comment.There's a typo in the comment: "wabacon" should be "way".
- // Format the services and commands in a readable wabacon + // Format the services and commands in a readable way
60-84: Consider extracting the blueprint lookup logic.The blueprint lookup and error handling could be extracted into a helper function for reusability and better testability.
+async fn get_blueprint_by_name( + context: &AppContext, + blueprint_name: &str, +) -> anyhow::Result<(AppBlueprintList, &AppBlueprint)> { + let result = get(context.server(), "blueprints").await?; + let blueprints: AppBlueprintList = serde_json::from_value(result)?; + + let blueprint = blueprints + .blueprints + .get(blueprint_name) + .ok_or_else(|| { + anyhow::anyhow!("Blueprint {} not found", blueprint_name.yellow()) + })?; + + Ok((blueprints, blueprint)) +}Then in
blueprint_info:- let result = get(context.server(), "blueprints").await?; - let blueprints: AppBlueprintList = serde_json::from_value(result)?; - - // Find the specified blueprint - let blueprint = blueprints - .blueprints - .get(&cmd.blueprint_name) - .ok_or_else(|| { - anyhow::anyhow!("Blueprint {} not found", cmd.blueprint_name.yellow()) - })?; + let (_blueprints, blueprint) = get_blueprint_by_name(context, &cmd.blueprint_name).await?;
120-142: Potential performance issue: Consider using references.The cloning of
ActionNamein the loop could be optimized by using references where possible.- let action_name: String = action.clone().into(); - let action_type: String = action.clone().get_type().into(); + let action_name: String = action.into(); + let action_type: String = action.get_type().into();And update the method signature to take
&ActionName:- for action in lifecycle_actions { + for action in &lifecycle_actions {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scotty-core/src/settings/app_blueprint.rs(3 hunks)scottyctl/src/commands/blueprints.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Suite
- GitHub Check: Test Suite
🔇 Additional comments (9)
scottyctl/src/commands/blueprints.rs (3)
1-1: LGTM! Imports are appropriate for the new functionality.The new imports support the enhanced blueprint information display with color formatting and the new command structure.
Also applies to: 3-3, 9-10
36-37: Good improvement to table column widths.The increased widths for description (25) and required services (40) columns will provide better readability for longer content.
145-160: Good separation of lifecycle and custom actions.The logic correctly separates lifecycle actions from custom actions and provides appropriate styling (dimmed for lifecycle, green for custom).
scotty-core/src/settings/app_blueprint.rs (6)
5-24: Well-designed ActionType enum with good conversion methods.The enum provides clear categorization between lifecycle and custom actions, with convenient string conversion methods.
77-86: Good design: Type introspection method.The
get_type()method provides clean categorization of action types without exposing internal enum details.
90-108: Well-structured Action replacement for nested HashMap.The
Actionstruct provides a clean abstraction with descriptive fields and appropriate helper methods. The optional description field with proper serde handling is a good design choice.
138-156: Excellent convenience methods for action access.The new methods provide clean, safe access to action data with appropriate Option handling. The method names are descriptive and follow Rust conventions.
175-187: Improved validation logic for new structure.The validation correctly adapts to the new
Actionstruct, ensuring all services referenced in action commands are properly declared in required services.
136-136: Good addition: Standard Error trait implementation.Implementing
std::error::ErrorforAppBlueprintValidationErrorimproves error handling integration with the Rust ecosystem.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scottyctl/src/commands/blueprints.rs (1)
45-58: Minor naming improvement: Consider pluralizing the function name.The helper function addresses the code duplication issue identified in previous reviews, which is excellent. However, the function name could be more accurate since it handles multiple commands per service.
Consider this small naming improvement:
-fn format_services_command(commands: &HashMap<String, Vec<String>>) -> String { +fn format_services_commands(commands: &HashMap<String, Vec<String>>) -> String {And update the calls accordingly:
-let services_commands = format_services_command(&action_obj.commands); +let services_commands = format_services_commands(&action_obj.commands);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scottyctl/src/commands/blueprints.rs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
scottyctl/src/commands/blueprints.rs (4)
scottyctl/src/api.rs (1)
get(153-155)scottyctl/src/context.rs (2)
ui(29-31)new(21-26)scotty-core/src/settings/app_blueprint.rs (3)
new(102-107)get_type(78-85)as_str(12-17)scottyctl/src/utils/ui.rs (1)
new(28-57)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: frontend
- GitHub Check: Clippy
- GitHub Check: Check
- GitHub Check: Test Suite
- GitHub Check: Check
- GitHub Check: Clippy
- GitHub Check: Test Suite
🔇 Additional comments (8)
scottyctl/src/commands/blueprints.rs (8)
1-2: LGTM: Required import added for HashMap usage.The HashMap import is correctly added to support the new functionality in the helper function and blueprint info display.
9-10: LGTM: Imports updated for new blueprint info functionality.The new imports for
BlueprintInfoCommandandActionNameare appropriately added to support the blueprint info command implementation.
36-37: LGTM: Table column widths adjusted for better readability.The width adjustments for the description and required services columns improve the display layout for the blueprint list.
60-84: LGTM: Well-structured function setup and basic info display.The function correctly initializes the UI status, fetches blueprint data, finds the specific blueprint with proper error handling, and formats the basic blueprint information with appropriate styling.
92-101: LGTM: Public services display with clean table formatting.The optional public services section is well-implemented with proper conditional rendering and clean table formatting using the tabled library.
103-142: LGTM: Comprehensive lifecycle actions handling.The lifecycle actions section correctly:
- Defines the standard lifecycle actions
- Sets up the table with appropriate headers
- Handles missing actions gracefully
- Formats action information with proper type conversion
- Uses the helper function to display commands consistently
144-160: LGTM: Clean separation of custom actions from lifecycle actions.The custom actions handling correctly filters for
ActionName::Customvariants and displays them with distinct styling (green coloring) while reusing the same formatting logic.
162-184: LGTM: Professional table styling and helpful user guidance.The final section applies consistent styling to the actions table and provides valuable user guidance on how to execute custom actions through the CLI. The success message and return format are appropriate.
Corrects the function call to properly format services commands, ensuring the operations within blueprints are executed accurately without errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scottyctl/src/commands/blueprints.rs (1)
120-142: Suggest improvements for more idiomatic Rust.The logic is correct but can be made more idiomatic:
- for action in &lifecycle_actions { - let action_obj = blueprint.actions.get(action); - if action_obj.is_none() { - continue; - } - let action_obj = action_obj.unwrap(); + for action in &lifecycle_actions { + let Some(action_obj) = blueprint.actions.get(action) else { + continue; + }; let action_name: String = action.clone().into(); let action_type: String = action.get_type().into(); - let description = match &action_obj.description { - desc if desc.is_empty() => action_type.as_str(), - desc => desc.as_str(), - }; + let description = if action_obj.description.is_empty() { + action_type.as_str() + } else { + action_obj.description.as_str() + }; // Format the services and commands in a readable way let services_commands = format_services_commands(&action_obj.commands); builder.push_record(vec![ - &action_name.dimmed().to_string(), + &action_name.dimmed(), &action_type, description, &services_commands, ]); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scottyctl/src/commands/blueprints.rs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
scottyctl/src/commands/blueprints.rs (3)
scottyctl/src/api.rs (1)
get(153-155)scottyctl/src/context.rs (2)
ui(29-31)new(21-26)scottyctl/src/utils/ui.rs (1)
new(28-57)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Check
- GitHub Check: Clippy
- GitHub Check: Test Suite
- GitHub Check: Clippy
- GitHub Check: Test Suite
- GitHub Check: Check
🔇 Additional comments (4)
scottyctl/src/commands/blueprints.rs (4)
1-2: LGTM! Import additions are appropriate.The new imports are correctly added and necessary for the new blueprint info functionality.
Also applies to: 9-10
36-37: Good improvement to table column widths.The increased widths for description and required services columns will improve readability.
45-58: Helper function successfully addresses code duplication.This function effectively extracts the common formatting logic for services and commands. The implementation is clean and follows good formatting practices with proper color styling and indentation.
Note: The past review comment suggested including a
dimmedparameter, but the current implementation doesn't include it. This appears to be intentional since the dimming is applied at the action level rather than the command level.
144-184: Excellent implementation of blueprint info display.The custom actions handling and overall function implementation provide:
- Clear separation between lifecycle and custom actions
- Appropriate color coding for different action types
- Helpful user guidance with the CLI usage note
- Consistent styling and good error handling
- Comprehensive blueprint information display
This is a well-implemented feature that enhances the CLI user experience.
Enable defining and running custom actions beyond standard lifecycle hooks. Users can now trigger these actions via the new API endpoint and CLI command. Blueprint files can define named actions that can be executed on demand.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation