[FIX] cetmix_tower_server: Server from template #190
Conversation
WalkthroughThe pull request introduces changes to the SSH key handling in the Changes
✨ Finishing Touches
🪧 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: 1
🧹 Nitpick comments (1)
cetmix_tower_server/readme/USAGE.md (1)
21-21: Fix typo in parameter documentation.There's a typo in the word "Referenct" which should be "Reference".
- - ssh_key (Char, optional): Referenct to SSH private key record. Defaults to None. + - ssh_key (Char, optional): Reference to SSH private key record. Defaults to None.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cetmix_tower_server/models/cx_tower_server_template.py(4 hunks)cetmix_tower_server/readme/USAGE.md(3 hunks)cetmix_tower_server/views/cx_tower_key_view.xml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (4)
cetmix_tower_server/models/cx_tower_server_template.py (2)
165-166: LGTM! Documentation updated correctly.The parameter documentation clearly describes that
ssh_keyis a reference to an SSH private key record.
191-192: LGTM! Documentation is consistent.The parameter documentation matches the parent method
create_server_from_template.cetmix_tower_server/views/cx_tower_key_view.xml (1)
13-13: LGTM! View changes align with the model updates.Making the reference field always visible is consistent with the new SSH key lookup by reference functionality.
cetmix_tower_server/readme/USAGE.md (1)
35-35: LGTM! Code formatting improved.The removal of unnecessary blank lines improves readability.
Also applies to: 48-49, 55-55
GabbasovDinar
force-pushed
the
14.0-t4272-cetmix_tower_server-fix-server-create
branch
from
54b4d20 to
4a6509c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cetmix_tower_server/models/cx_tower_server_template.py (1)
382-422: Optimize server config parsing using dict comprehension.The method is well-structured but could be more concise and efficient.
Consider this more efficient implementation:
def _parse_server_configs(self, config_values): """ Prepares server configuration values. Args: config_values (dict): A dictionary containing server configuration values. Keys and their expected values: - partner (res.partner, optional): The partner this server belongs to. - ipv4 (str, optional): IPv4 address. Defaults to None. - ipv6 (str, optional): IPv6 address. Must be provided if IPv4 is not specified. Defaults to None. - ssh_key (str, optional): Reference to an SSH private key record. Defaults to None. Returns: dict: A dictionary containing parsed server configuration values with the following keys: - partner_id (int, optional): ID of the partner. - ssh_key_id (int, optional): ID of the associated SSH key. - ip_v4_address (str, optional): Parsed IPv4 address. - ip_v6_address (str, optional): Parsed IPv6 address. """ - values = {} - - if config_values.get("partner"): - values["partner_id"] = config_values.pop("partner").id - - if config_values.get("ssh_key"): - ssh_key_reference = config_values.pop("ssh_key") - ssh_key = self.env["cx.tower.key"].get_by_reference(ssh_key_reference) - if ssh_key: - values["ssh_key_id"] = ssh_key.id - - if config_values.get("ipv4"): - values["ip_v4_address"] = config_values.pop("ipv4") - - if config_values.get("ipv6"): - values["ip_v6_address"] = config_values.pop("ipv6") - - return values + values = { + "partner_id": config_values.pop("partner").id + if config_values.get("partner") + else False, + "ip_v4_address": config_values.pop("ipv4", False), + "ip_v6_address": config_values.pop("ipv6", False), + } + + # Handle SSH key separately due to reference lookup + if config_values.get("ssh_key"): + ssh_key = self.env["cx.tower.key"].get_by_reference( + config_values.pop("ssh_key") + ) + values["ssh_key_id"] = ssh_key.id if ssh_key else False + + return {k: v for k, v in values.items() if v is not False}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cetmix_tower_server/models/cx_tower_server_template.py(4 hunks)cetmix_tower_server/readme/USAGE.md(3 hunks)cetmix_tower_server/views/cx_tower_key_view.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cetmix_tower_server/views/cx_tower_key_view.xml
- cetmix_tower_server/readme/USAGE.md
🧰 Additional context used
📓 Learnings (1)
cetmix_tower_server/models/cx_tower_server_template.py (1)
Learnt from: GabbasovDinar
PR: cetmix/cetmix-tower#190
File: cetmix_tower_server/models/cx_tower_server_template.py:275-280
Timestamp: 2025-01-21T12:35:34.636Z
Learning: In the `cx.tower.server.template` model, the SSH key parameter is optional when creating a server from template, as servers can be created with password authentication instead.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (1)
cetmix_tower_server/models/cx_tower_server_template.py (1)
165-167: Documentation updates look good!The parameter documentation is clear, consistent, and correctly reflects the optional nature of the SSH key parameter.
Also applies to: 191-193
GabbasovDinar
force-pushed
the
14.0-t4272-cetmix_tower_server-fix-server-create
branch
from
b692c60 to
369735d
Compare
- Fix using server configuration when creating a server from a template Task: 4272
GabbasovDinar
force-pushed
the
14.0-t4272-cetmix_tower_server-fix-server-create
branch
from
369735d to
597ed5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cetmix_tower_server/models/cx_tower_server_template.py (1)
417-423: Consider adding IP address format validation.The IPv4 and IPv6 addresses are assigned without format validation. Consider adding validation to ensure the addresses are in the correct format.
ipv4 = config_values.pop("ipv4", None) if ipv4: + # Validate IPv4 format (x.x.x.x) + if not re.match(r'^(\d{1,3}\.){3}\d{1,3}$', ipv4): + raise ValidationError(_("Invalid IPv4 address format")) values["ip_v4_address"] = ipv4 ipv6 = config_values.pop("ipv6", None) if ipv6: + # Validate IPv6 format + if not re.match(r'^([0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}$', ipv6): + raise ValidationError(_("Invalid IPv6 address format")) values["ip_v6_address"] = ipv6Don't forget to add the import at the top of the file:
import re
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cetmix_tower_server/models/cx_tower_server_template.py(4 hunks)cetmix_tower_server/readme/USAGE.md(3 hunks)cetmix_tower_server/views/cx_tower_key_view.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cetmix_tower_server/readme/USAGE.md
- cetmix_tower_server/views/cx_tower_key_view.xml
🧰 Additional context used
📓 Learnings (1)
cetmix_tower_server/models/cx_tower_server_template.py (2)
Learnt from: GabbasovDinar
PR: cetmix/cetmix-tower#190
File: cetmix_tower_server/models/cx_tower_server_template.py:275-277
Timestamp: 2025-01-22T05:46:25.910Z
Learning: In the `CxTowerServerTemplate` class, server config values (partner_id, ssh_key_id, ip_v4_address, ip_v6_address) parsed from kwargs are template-level configurations that should be consistent across all servers created from that template. Therefore, these values should be parsed once before processing individual server records.
Learnt from: GabbasovDinar
PR: cetmix/cetmix-tower#190
File: cetmix_tower_server/models/cx_tower_server_template.py:275-280
Timestamp: 2025-01-21T12:35:34.636Z
Learning: In the `cx.tower.server.template` model, the SSH key parameter is optional when creating a server from template, as servers can be created with password authentication instead.
🔇 Additional comments (3)
cetmix_tower_server/models/cx_tower_server_template.py (3)
165-167: Documentation updates look good!The docstrings are clear, consistent, and accurately reflect the parameter changes.
Also applies to: 191-193
275-277: LGTM! Good separation of concerns.Moving server config parsing to a separate method improves code organization. The placement before processing individual server records is correct as these are template-level configurations.
382-425: Implementation looks clean and efficient!The method is well-documented and follows the performance improvements suggested in past reviews. The SSH key handling is correctly implemented as optional.
|
/ocabot merge patch |
|
What a great day to merge this nice PR. Let's do it! |
|
Congratulations, your PR was merged at 3ff4f25. Thanks a lot for contributing to cetmix. ❤️ |
CetmixGitDrone
deleted the
14.0-t4272-cetmix_tower_server-fix-server-create
branch
Task: 4272
Summary by CodeRabbit
New Features
Documentation
User Interface