EDM-2929: Add Enum-to-string conversion to Flightctl inventory plugin #51
Conversation
WalkthroughAdds a recursive helper to convert Enum values to strings and applies it when preparing device data for inventory population, ensuring Enum members are stringified before validation and insertion. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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[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 (1)
plugins/inventory/flightctl.py (1)
661-673: Consider preserving tuple types and handling sets.The function correctly converts Enum values, but has two minor type-handling gaps:
- Tuple preservation: Lines 670-671 convert tuples to lists. While Ansible typically uses lists for variables, preserving the original type would be more accurate.
- Set handling: Line 408 checks for
settypes, but this function doesn't handle them. If a device field contains a set with Enum values, they won't be converted.🔎 Proposed improvements
def _convert_enums_to_strings(obj: Any) -> Any: """ Recursively convert all Enum values in a data structure to their string values. This is needed because Ansible's inventory.set_variable() doesn't support Enum types. """ if isinstance(obj, Enum): return obj.value if isinstance(obj, dict): return {k: _convert_enums_to_strings(v) for k, v in obj.items()} - if isinstance(obj, (list, tuple)): + if isinstance(obj, list): return [_convert_enums_to_strings(item) for item in obj] + if isinstance(obj, tuple): + return tuple(_convert_enums_to_strings(item) for item in obj) + if isinstance(obj, set): + return {_convert_enums_to_strings(item) for item in obj} return obj
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/inventory/flightctl.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: SiddarthR56
Repo: flightctl/flightctl-ansible PR: 32
File: tests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.yml:48-49
Timestamp: 2025-04-24T17:24:38.607Z
Learning: The assertions checking CSR approval status in tests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.yml are temporarily commented out pending the merge of PR #1104 (https://github.com/flightctl/flightctl/pull/1104) which addresses an issue with CSR approval endpoints. These should be re-enabled once that PR is merged.
Learnt from: gshilin-sdb
Repo: flightctl/flightctl-ansible PR: 31
File: plugins/inventory/flightctl.py:694-712
Timestamp: 2025-04-28T12:30:07.851Z
Learning: In the flightctl.core.flightctl inventory plugin, logging errors via display.error() and continuing execution is preferred over raising exceptions for non-critical configuration issues, to maintain plugin resilience and allow it to function with partial data.
Learnt from: gshilin-sdb
Repo: flightctl/flightctl-ansible PR: 34
File: plugins/inventory/flightctl.py:288-291
Timestamp: 2025-05-06T08:44:35.361Z
Learning: In flightctl.py inventory plugin, authentication credentials (access_token or username/password) are only required when SSL verification is enabled (verify_ssl=True). When SSL verification is disabled, the system allows unauthenticated requests by design.
Learnt from: gshilin-sdb
Repo: flightctl/flightctl-ansible PR: 31
File: plugins/inventory/flightctl.py:0-0
Timestamp: 2025-04-22T13:45:41.777Z
Learning: The _get_data function in flightctl.py inventory plugin is intentionally designed to fetch ALL devices/fleets without filtering via label_selector or field_selector parameters. Filtering is handled by the plugin's logic after retrieving the complete dataset.
Learnt from: gshilin-sdb
Repo: flightctl/flightctl-ansible PR: 31
File: plugins/inventory/flightctl.py:232-251
Timestamp: 2025-04-28T11:44:07.573Z
Learning: In the flightctl inventory plugin, authentication validation (token/username/password) is only required when SSL verification is enabled (verify_ssl=True). This is intentional behavior per the author's design.
Learnt from: gshilin-sdb
Repo: flightctl/flightctl-ansible PR: 31
File: plugins/inventory/flightctl.py:0-0
Timestamp: 2025-04-22T13:45:42.302Z
Learning: The _get_data helper function in plugins/inventory/flightctl.py only accepts two parameters: list_func (a callable) and an optional limit parameter. It doesn't support directly passing additional parameters like label_selector or field_selector to the API functions.
📚 Learning: 2025-04-28T12:30:07.851Z
Learnt from: gshilin-sdb
Repo: flightctl/flightctl-ansible PR: 31
File: plugins/inventory/flightctl.py:694-712
Timestamp: 2025-04-28T12:30:07.851Z
Learning: In the flightctl.core.flightctl inventory plugin, logging errors via display.error() and continuing execution is preferred over raising exceptions for non-critical configuration issues, to maintain plugin resilience and allow it to function with partial data.
Applied to files:
plugins/inventory/flightctl.py
📚 Learning: 2025-04-22T13:45:41.777Z
Learnt from: gshilin-sdb
Repo: flightctl/flightctl-ansible PR: 31
File: plugins/inventory/flightctl.py:0-0
Timestamp: 2025-04-22T13:45:41.777Z
Learning: The _get_data function in flightctl.py inventory plugin is intentionally designed to fetch ALL devices/fleets without filtering via label_selector or field_selector parameters. Filtering is handled by the plugin's logic after retrieving the complete dataset.
Applied to files:
plugins/inventory/flightctl.py
📚 Learning: 2025-05-06T08:44:35.361Z
Learnt from: gshilin-sdb
Repo: flightctl/flightctl-ansible PR: 34
File: plugins/inventory/flightctl.py:288-291
Timestamp: 2025-05-06T08:44:35.361Z
Learning: In flightctl.py inventory plugin, authentication credentials (access_token or username/password) are only required when SSL verification is enabled (verify_ssl=True). When SSL verification is disabled, the system allows unauthenticated requests by design.
Applied to files:
plugins/inventory/flightctl.py
📚 Learning: 2025-04-28T11:44:07.573Z
Learnt from: gshilin-sdb
Repo: flightctl/flightctl-ansible PR: 31
File: plugins/inventory/flightctl.py:232-251
Timestamp: 2025-04-28T11:44:07.573Z
Learning: In the flightctl inventory plugin, authentication validation (token/username/password) is only required when SSL verification is enabled (verify_ssl=True). This is intentional behavior per the author's design.
Applied to files:
plugins/inventory/flightctl.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-unit-downstream
- GitHub Check: test-sanity-downstream
- GitHub Check: integration-tests (stable-2.16, 3.12)
🔇 Additional comments (6)
plugins/inventory/flightctl.py (6)
45-48: LGTM!The organization option is clearly documented and follows the established pattern for other configuration options.
139-139: LGTM!The Enum import is necessary for the new
_convert_enums_to_stringshelper function and properly placed in the imports section.
323-324: LGTM!The Enum-to-string conversion is correctly applied after
to_dict()conversion and before device validation. The comment clearly explains the purpose.
341-342: LGTM!Consistent application of Enum conversion in the main device population path.
389-390: LGTM!Enum conversion is consistently applied in both static and keyed additional group processing paths, ensuring complete coverage across all inventory population mechanisms.
Also applies to: 402-403
281-283: No action needed—Configuration.organizationis a valid attribute.The organization option parsing and assignment are correct. The attribute is documented in the plugin's options (lines 45-47), and the setting pattern on line 303 matches the established approach used for other optional configuration attributes like
request_timeoutandverify_ssl.
amalykhi
changed the title
amalykhi
changed the title
amalykhi
force-pushed
the
inventory-test-fix
branch
from
3718c0c to
af4ea2b
Compare
amalykhi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugins/inventory/flightctl.py (1)
661-673: Well-implemented solution to the Ansible Enum limitation.The recursive helper correctly converts Enum values to strings across all nested data structures. The docstring clearly explains the purpose, and the implementation handles the common cases (dict, list, tuple, Enum).
Optional: Note on tuple-to-list conversion
The function converts tuples to lists (line 671), which changes immutability semantics. This is acceptable since Ansible inventory variables expect JSON-serializable types, but if preserving tuple types is desired, you could modify line 671:
if isinstance(obj, (list, tuple)): - return [_convert_enums_to_strings(item) for item in obj] + converted = [_convert_enums_to_strings(item) for item in obj] + return type(obj)(converted) if isinstance(obj, tuple) else convertedHowever, this is likely unnecessary for the inventory use case.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/inventory/flightctl.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-04-22T13:45:41.777Z
Learnt from: gshilin-sdb
Repo: flightctl/flightctl-ansible PR: 31
File: plugins/inventory/flightctl.py:0-0
Timestamp: 2025-04-22T13:45:41.777Z
Learning: The _get_data function in flightctl.py inventory plugin is intentionally designed to fetch ALL devices/fleets without filtering via label_selector or field_selector parameters. Filtering is handled by the plugin's logic after retrieving the complete dataset.
Applied to files:
plugins/inventory/flightctl.py
📚 Learning: 2025-04-28T12:30:07.851Z
Learnt from: gshilin-sdb
Repo: flightctl/flightctl-ansible PR: 31
File: plugins/inventory/flightctl.py:694-712
Timestamp: 2025-04-28T12:30:07.851Z
Learning: In the flightctl.core.flightctl inventory plugin, logging errors via display.error() and continuing execution is preferred over raising exceptions for non-critical configuration issues, to maintain plugin resilience and allow it to function with partial data.
Applied to files:
plugins/inventory/flightctl.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-unit-downstream
- GitHub Check: test-sanity-downstream
- GitHub Check: integration-tests (stable-2.16, 3.12)
🔇 Additional comments (2)
plugins/inventory/flightctl.py (2)
139-139: LGTM!The
Enumimport is necessary for the new helper function and correctly placed with other standard library imports.
323-324: Excellent consistent application across all device processing paths.The enum-to-string conversion is correctly applied at all four locations where device data enters the inventory:
- Fleet devices (lines 323-324)
- Direct devices (lines 341-342)
- Additional groups (static) (lines 389-390)
- Additional groups (keyed) (lines 402-403)
The consistent pattern—convert to dict, convert enums, then validate—ensures that all devices have Enum values converted to strings before being stored in Ansible inventory variables. The explanatory comments are helpful and uniform across all locations.
Also applies to: 341-342, 389-390, 402-403
amalykhi
force-pushed
the
inventory-test-fix
branch
from
af4ea2b to
7c7bce5
Compare
2 participants
Fixing the error
**Type 'ConditionType' is unsupported for variable storage.
Origin:
ConditionType.DeviceUpdating**
After fix
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.