Conversation
|
I have read the CLA Document and I hereby sign the CLA zghp seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for loading slot configuration from external YAML or JSON files when adding slots to the allocator server.
- Added a new optional
--configargument to theadd-slotcommand that accepts a file path - Implemented logic to parse YAML and JSON configuration files and include them in the slot creation payload
- Removed an unnecessary blank line for code formatting
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| configuration = None | ||
| if config: | ||
| import json, yaml |
There was a problem hiding this comment.
The yaml module is already imported at the top of the file (line 8), so it doesn't need to be imported again here. Only json needs to be imported locally. Consider moving the json import to the top of the file with other imports, or change this line to import json only.
|
|
||
| if configuration: | ||
| payload['configuration'] = configuration | ||
|
|
There was a problem hiding this comment.
Trailing whitespace detected. Please remove the trailing spaces at the end of this line.
| Add a new slot to the allocator server. | ||
|
|
||
| Args: | ||
| args (list): Command-line arguments. |
There was a problem hiding this comment.
The docstring is outdated. It states "Args: args (list): Command-line arguments" but the function signature shows multiple specific parameters. Please update the docstring to document all parameters: server, rack_name, slot_name, platform, owner_email, state, description, tags, and the newly added config.
| args (list): Command-line arguments. | |
| server (str): Allocator server URL. | |
| rack_name (str): Name of the rack. | |
| slot_name (str): Name of the slot. | |
| platform (str): Platform type for the slot. | |
| owner_email (str or None): Email address of the slot owner. | |
| state (str, optional): State of the slot. Defaults to ''. | |
| description (str, optional): Description of the slot. Defaults to ''. | |
| tags (list of str, optional): List of tags for the slot. Defaults to []. | |
| config (str or None, optional): Path to a configuration file (YAML or JSON) for the slot. Defaults to None. |
| plugin_utils.warning(f"Unsupported config format for file: {config}") | ||
| except Exception as e: | ||
| plugin_utils.error(f"Failed to load configuration file: {e}") | ||
| configuration = None |
There was a problem hiding this comment.
This line is unreachable code. The plugin_utils.error() function raises SystemExit(1) and terminates the program, so configuration = None will never be executed. Consider removing this line.
| configuration = None |
| except Exception as e: | ||
| plugin_utils.error(f"Failed to load configuration file: {e}") | ||
| configuration = None |
There was a problem hiding this comment.
Consider catching more specific exceptions instead of the broad Exception. For file operations, FileNotFoundError, PermissionError, and IOError are more appropriate. For parsing errors, yaml.YAMLError and json.JSONDecodeError should be caught separately to provide more specific error messages to the user.
| except Exception as e: | |
| plugin_utils.error(f"Failed to load configuration file: {e}") | |
| configuration = None | |
| except FileNotFoundError as e: | |
| plugin_utils.error(f"Configuration file not found: {e}") | |
| configuration = None | |
| except PermissionError as e: | |
| plugin_utils.error(f"Permission denied when accessing configuration file: {e}") | |
| configuration = None | |
| except IOError as e: | |
| plugin_utils.error(f"I/O error when accessing configuration file: {e}") | |
| configuration = None | |
| except yaml.YAMLError as e: | |
| plugin_utils.error(f"YAML parsing error in configuration file: {e}") | |
| configuration = None | |
| except json.JSONDecodeError as e: | |
| plugin_utils.error(f"JSON parsing error in configuration file: {e}") | |
| configuration = None |
| payload['configuration'] = configuration | ||
|
|
There was a problem hiding this comment.
Consider validating that the loaded configuration is a dictionary before adding it to the payload. Both yaml.safe_load() and json.load() can return various types (None, lists, strings, etc.) depending on the file content. If the configuration is not a dictionary, it could cause issues with the API endpoint.
| payload['configuration'] = configuration | |
| if isinstance(configuration, dict): | |
| payload['configuration'] = configuration | |
| else: | |
| plugin_utils.warning(f"Configuration loaded from {config} is not a dictionary and will be ignored.") |
1 participant
No description provided.