Fix Local Deployment Invocation
#231
Conversation
Signed-off-by: Muhammad Mahad <[email protected]>
- Add required positional arguments — architecture option and container deployment
- Add `local` deployment check in supported languages versions retrieval
Signed-off-by: Muhammad Mahad <[email protected]>
WalkthroughThe pull request introduces enhancements to the SeBs (Serverless Benchmarking System) project, focusing on three key files: Changes
Sequence DiagramsequenceDiagram
participant CLI as Command Line Interface
participant Config as SeBSConfig
participant Deployment as Deployment Module
CLI->>+Config: Get supported language versions
Config-->>-CLI: Return supported versions
CLI->>Deployment: Start deployment
Deployment-->>CLI: Deployment result
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Nitpick comments (2)
sebs/config.py (1)
40-45: LGTM! The refactored code is more robust.The changes improve error handling by using safe dictionary access methods and adding special handling for local deployment.
Consider adding type hints for better code maintainability:
def supported_language_versions( self, deployment_name: str, language_name: str, architecture: str - ) -> List[str]: + ) -> List[str]: + languages: Dict[str, Any] = self._system_config.get(deployment_name, {}).get("languages", {}) + base_images: Dict[str, Any] = languages.get(language_name, {}).get("base_images", {})sebs/cache.py (1)
173-181: LGTM! Good defensive programming practice added.The changes prevent potential file not found errors and improve code maintainability by standardizing path handling.
Consider adding a debug log message when skipping due to missing config:
if self.ignore_storage or not os.path.exists(config_path): + self.logging.debug(f"Skipping storage update: ignore_storage={self.ignore_storage}, config exists={os.path.exists(config_path)}") return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
sebs.py(3 hunks)sebs/cache.py(1 hunks)sebs/config.py(1 hunks)
🔇 Additional comments (2)
sebs.py (2)
450-455: LGTM! Well-structured architecture option added.The option is properly defined with a sensible default and clear choices.
474-475: Verify the hardcoded parameter values.The hardcoded
container_deployment=Falseandignore_cache=Falseparameters might limit flexibility. Consider making these configurable if they need to vary in the future.Run this script to check if these parameters are always false in local deployment:
|
@MahadMuhammad Thank you very much for your help, this looks great! |
2 participants
architectureandcontainer_deploymentlocaldeployment check in supported languages versions retrieval.Summary by CodeRabbit
Release Notes
New Features
--architectureoption to thestartcommand, allowing users to specify target architecture for function instances (x64 or arm64)Improvements
Bug Fixes