EDM-2774: Update Collection to v1.0 APIs #50
Conversation
WalkthroughIntroduces organization-scoped requests and wiring across modules, config loader, inventory plugin, docs, demos, and tests; bumps example apiVersion to flightctl.io/v1beta1, updates token-selection logic, adds CI free-disk steps, and increments collection metadata to v1.4.0. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (7)
CHANGELOG.rst (1)
7-19: Verify API version naming and enhance changelog specificity.The changelog entries refer to "Flight Control API v1.0", but the PR summary mentions "API v1beta1". Additionally, the release notes are quite generic and don't document specific changes described in the PR summary, such as organization-scoped request support, API version defaults, and enhanced token/configuration handling.
🔎 Suggested enhanced changelog entries
v1.4.0 ====== Release Summary --------------- - Added support for Flight Control API v1.0. + Added support for Flight Control API v1beta1 with organization-scoped operations. Major Changes ↓ - - Added support for new features in Flight Control API v1.0. - - Updated the Ansible collection to support Flight Control API version 1.0. + - Added support for organization-scoped resources and operations. + - Updated default API version from v1alpha1 to v1beta1. + - Enhanced authentication and configuration handling for organization contexts.changelogs/changelog.yaml (1)
99-103: Minor: Trailing whitespace on line 103.The reformatting of the minor_changes entries looks good, but line 103 appears to contain trailing whitespace.
🔎 Suggested cleanup
- Inventory plugin now supports `group_by` option for grouping devices by a field value. - + release_summary: Added support for Flight Control API v0.10.0 and improved thetests/integration/targets/connection_flightctl_console/runme.sh (1)
12-18: Consider handling missingflightctl_organizationgracefully.When
flightctl_organizationis not present in the config file,grepreturns an empty string, resulting in-e "flightctl_organization=". This sets the variable to an empty string rather than leaving it undefined.In playbooks using
default(omit), an empty string is not the same as an undefined variable, which may cause unintended behavior if the module differentiates between the two.🔎 Proposed fix to conditionally pass the organization
FLIGHTCTL_ORGANIZATION=$(grep -oP 'flightctl_organization:\s*\K.*' "../../integration_config.yml") +ORG_ARGS="" +if [ -n "$FLIGHTCTL_ORGANIZATION" ]; then + ORG_ARGS="-e flightctl_organization=${FLIGHTCTL_ORGANIZATION}" +fi + ansible-playbook "./test_connection.yml" \ -e "flightctl_host=${FLIGHTCTL_HOST}" \ -e "flightctl_token=${FLIGHTCTL_TOKEN}" \ - -e "flightctl_organization=${FLIGHTCTL_ORGANIZATION}" \ + ${ORG_ARGS} \ "${CMD_ARGS[@]}"tests/integration/targets/inventory_flightctl/runme.sh (2)
24-24: Consider conditional organization field in inventory config.When
FLIGHTCTL_ORGANIZATIONis empty, this generatesorganization:(empty value) in the inventory config, which may not be equivalent to omitting the field entirely. Consider conditionally including the organization line only when the value is non-empty.🔎 Alternative approach using conditional config generation
# Create inventory configuration file with comprehensive additional groups for testing +ORG_LINE="" +if [ -n "${FLIGHTCTL_ORGANIZATION}" ]; then + ORG_LINE="organization: ${FLIGHTCTL_ORGANIZATION}" +fi + cat > .config/flightctl/inventory.yml <<EOF --- plugin: flightctl.core.flightctl verify_ssl: False host: ${FLIGHTCTL_HOST} token: ${FLIGHTCTL_TOKEN} -organization: ${FLIGHTCTL_ORGANIZATION} +${ORG_LINE} request_timeout: 120
89-89: Empty organization value may not work with default(omit) pattern.When
FLIGHTCTL_ORGANIZATIONis empty, passing-e "flightctl_organization="to the playbook creates a variable with an empty string value rather than leaving it undefined. This may not work correctly with thedefault(omit)pattern used in the playbooks, which expects the variable to be undefined to trigger omission.🔎 Proposed fix to conditionally pass organization parameter
+ORG_EXTRA_VAR="" +if [ -n "${FLIGHTCTL_ORGANIZATION}" ]; then + ORG_EXTRA_VAR="-e flightctl_organization=${FLIGHTCTL_ORGANIZATION}" +fi + echo "Step 2: Setting up test resources..." -ansible-playbook ./inventory_setup_test.yml -e "flightctl_host=${FLIGHTCTL_HOST}" -e "flightctl_token=${FLIGHTCTL_TOKEN}" -e "flightctl_organization=${FLIGHTCTL_ORGANIZATION}" "${CMD_ARGS[@]}" +ansible-playbook ./inventory_setup_test.yml -e "flightctl_host=${FLIGHTCTL_HOST}" -e "flightctl_token=${FLIGHTCTL_TOKEN}" ${ORG_EXTRA_VAR} "${CMD_ARGS[@]}" # Wait a bit for resources to be fully created echo "Waiting for resources to be ready..." sleep 5 echo "Step 3: Testing inventory discovery..." -ansible-playbook ./inventory_test.yml -i ./.config/flightctl/inventory.yml -e "flightctl_host=${FLIGHTCTL_HOST}" -e "flightctl_token=${FLIGHTCTL_TOKEN}" -e "flightctl_organization=${FLIGHTCTL_ORGANIZATION}" "${CMD_ARGS[@]}" +ansible-playbook ./inventory_test.yml -i ./.config/flightctl/inventory.yml -e "flightctl_host=${FLIGHTCTL_HOST}" -e "flightctl_token=${FLIGHTCTL_TOKEN}" ${ORG_EXTRA_VAR} "${CMD_ARGS[@]}"Also applies to: 96-96
plugins/module_utils/config_loader.py (2)
109-114: Consider explicit error handling when token-to-use references a missing token.The token resolution logic falls back to
access_tokenwhentoken-to-useis set to "refresh" butrefresh_tokenis missing. While this provides a graceful fallback, it might mask configuration errors where users expect a refresh token to be used but it's not present.Consider adding a warning or validation when
token-to-usespecifies a token type that isn't available:🔎 Suggested improvement
if token_to_use == "refresh" and refresh_token: setattr(self, "token", refresh_token) +elif token_to_use == "refresh" and not refresh_token: + # Log warning: refresh token requested but not available + pass # Falls through to access_token fallback elif token_to_use == "access" and access_token: setattr(self, "token", access_token) elif access_token: setattr(self, "token", access_token)
110-110: Consider replacing setattr with direct assignment for readability.The static analysis tool flags the use of
setattrwith constant attribute names. While this is consistent with the existing codebase style (see lines 127, 133, 136), direct assignment would be more readable and is the preferred pattern in modern Python:self.token = refresh_token # instead of setattr(self, "token", refresh_token)However, since this pattern is used throughout the file, this refactor should be applied consistently across the entire class if adopted.
Also applies to: 112-112, 114-114, 121-121
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
.github/workflows/integration-tests.yamlCHANGELOG.rstMakefileREADME.mdchangelogs/changelog.yamldemo/create.ymldemo/files/device.ymldemo/files/fleet.ymldemo/inventory/prepare-devices-and-fleets.ymlgalaxy.ymlplugins/doc_fragments/auth.pyplugins/inventory/flightctl.pyplugins/module_utils/api_module.pyplugins/module_utils/config_loader.pyplugins/module_utils/core.pyplugins/modules/flightctl_resource.pytests/integration/targets/connection_flightctl_console/runme.shtests/integration/targets/connection_flightctl_console/test_connection.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-denial.ymltests/integration/targets/flightctl_certificate_management/tasks/enrollment-approval.ymltests/integration/targets/flightctl_certificate_management/tasks/enrollment-denial.ymltests/integration/targets/flightctl_enrollment_config_info/tasks/fetch-config.ymltests/integration/targets/flightctl_resource/tasks/update-device.ymltests/integration/targets/flightctl_resource_info/tasks/devices.ymltests/integration/targets/flightctl_resource_info/tasks/fleets.ymltests/integration/targets/flightctl_resource_info/tasks/repository.ymltests/integration/targets/flightctl_resource_info/tasks/resource-sync.ymltests/integration/targets/inventory_flightctl/inventory_setup_test.ymltests/integration/targets/inventory_flightctl/inventory_test.ymltests/integration/targets/inventory_flightctl/runme.shtests/unit/plugins/module_utils/fixtures/client.yaml
🧰 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.
📚 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:
tests/unit/plugins/module_utils/fixtures/client.yamltests/integration/targets/connection_flightctl_console/test_connection.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-denial.ymltests/integration/targets/inventory_flightctl/runme.shtests/integration/targets/flightctl_enrollment_config_info/tasks/fetch-config.ymlplugins/doc_fragments/auth.pyplugins/module_utils/core.pytests/integration/targets/inventory_flightctl/inventory_setup_test.ymlplugins/inventory/flightctl.pytests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.ymldemo/inventory/prepare-devices-and-fleets.ymldemo/create.yml
📚 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:
tests/unit/plugins/module_utils/fixtures/client.yamltests/integration/targets/inventory_flightctl/runme.shtests/integration/targets/inventory_flightctl/inventory_setup_test.ymlplugins/inventory/flightctl.py
📚 Learning: 2025-04-24T17:24:38.607Z
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.
Applied to files:
tests/integration/targets/flightctl_resource_info/tasks/resource-sync.ymltests/integration/targets/inventory_flightctl/inventory_test.ymltests/integration/targets/flightctl_resource_info/tasks/devices.ymltests/integration/targets/flightctl_certificate_management/tasks/enrollment-denial.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-denial.ymltests/integration/targets/connection_flightctl_console/runme.sh.github/workflows/integration-tests.yamltests/integration/targets/flightctl_resource_info/tasks/repository.ymltests/integration/targets/inventory_flightctl/runme.shtests/integration/targets/flightctl_enrollment_config_info/tasks/fetch-config.ymltests/integration/targets/flightctl_certificate_management/tasks/enrollment-approval.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.ymltests/integration/targets/flightctl_resource_info/tasks/fleets.yml
📚 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
🧬 Code graph analysis (2)
plugins/modules/flightctl_resource.py (1)
plugins/module_utils/resources.py (1)
api_version(31-32)
plugins/module_utils/config_loader.py (1)
plugins/module_utils/api_module.py (1)
get(178-212)
🪛 Ruff (0.14.8)
plugins/module_utils/config_loader.py
109-109: Possible hardcoded password assigned to: "token_to_use"
(S105)
110-110: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
111-111: Possible hardcoded password assigned to: "token_to_use"
(S105)
112-112: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
114-114: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
121-121: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
⏰ 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). (2)
- GitHub Check: test-unit-downstream
- GitHub Check: test-sanity-downstream
🔇 Additional comments (38)
galaxy.yml (1)
11-11: Version bump is correctly applied.The collection version is properly incremented from 1.3.0 to 1.4.0, following semantic versioning conventions and aligning with the PR scope.
CHANGELOG.rst (1)
38-38: Verify formatting intent for blank line before v1.3.0 Bugfixes.An extra blank line was inserted before the Bugfixes section (line 38). Confirm whether this is intentional for consistency with other release sections or an accidental edit.
changelogs/changelog.yaml (1)
104-108: LGTM!The reformatting of the release_summary and release_date for version 1.3.0 maintains proper YAML formatting.
tests/integration/targets/flightctl_certificate_management/tasks/enrollment-approval.yml (2)
8-8: LGTM! Organization support added with backward compatibility.The addition of
flightctl_organizationwithdefault(omit)ensures backward compatibility while enabling organization-scoped requests. This pattern is consistent across all test files in the PR.
16-16: Verify v1beta1 API version support in FlightCTL v1.0.0.The test file updates the EnrollmentRequest API version from
flightctl.io/v1alpha1toflightctl.io/v1beta1. Public documentation and examples consistently reference only v1alpha1. Confirm that FlightCTL v1.0.0 officially supports v1beta1 and document any breaking changes between API versions..github/workflows/integration-tests.yaml (1)
14-14: LGTM! Workflow updated to use FlightCTL v1.0.0.The
FLIGHTCTL_REFupdate from v0.10.0 to v1.0.0 aligns with the broader API version migration in this PR. This ensures integration tests run against the correct FlightCTL version.demo/files/device.yml (1)
2-2: LGTM! Demo manifest updated to v1beta1 API.The API version update is consistent with the broader migration across the repository.
tests/integration/targets/flightctl_certificate_management/tasks/enrollment-denial.yml (1)
8-8: LGTM! Consistent updates for organization support and API version.Both the organization parameter addition and API version update follow the same pattern as other certificate management tests, ensuring consistency across the test suite.
Also applies to: 16-16
tests/integration/targets/flightctl_enrollment_config_info/tasks/fetch-config.yml (1)
8-8: LGTM! Enrollment config test updated consistently.The organization support and API version update align with the changes across all integration tests.
Also applies to: 16-16
demo/files/fleet.yml (1)
2-2: LGTM! Fleet manifest updated to v1beta1 API.Consistent with the Device manifest update in the demo files.
tests/integration/targets/connection_flightctl_console/test_connection.yml (1)
11-11: LGTM! Connection plugin updated with organization support.The addition of
ansible_flightctl_organizationfollows the correct naming convention for connection plugin variables (with theansible_prefix) and maintains backward compatibility withdefault(omit).tests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.yml (1)
8-8: LGTM! Certificate signing test updated consistently.The organization support and API version update maintain consistency with the other certificate management tests in this PR.
Also applies to: 16-16
tests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-denial.yml (2)
8-8: LGTM!The addition of
flightctl_organizationtoconnection_infois consistent with the PR's objective to support organization-scoped requests. Thedefault(omit)pattern correctly makes this optional.
16-16: API version bump looks correct.The update from
v1alpha1tov1beta1aligns with the broader migration to the v1beta1 API across the collection.plugins/doc_fragments/auth.py (1)
33-37: LGTM!The documentation for
flightctl_organizationis well-structured and follows the established pattern for other options in this fragment. The environment variable fallback (FLIGHTCTL_ORGANIZATION) is consistent with the naming convention used throughout.README.md (2)
59-65: Version references updated consistently.The version bump to
1.4.0in both the requirements.yml example and the command-line installation example is consistent.
96-101: Good addition of organization-scoped example.The new example clearly demonstrates how to use
flightctl_organizationto scope resources to a specific organization, which is helpful for users adopting this new feature.demo/inventory/prepare-devices-and-fleets.yml (2)
14-14: LGTM!The addition of
flightctl_organizationtocredential_defaultsfollows the established pattern withdefault(omit)for optional handling.
27-27: API version updates are consistent.All Device and Fleet resource definitions have been updated from
v1alpha1tov1beta1, which aligns with the PR's migration objectives.Also applies to: 48-48, 69-69, 91-91, 111-111, 131-131, 150-150, 202-202
demo/create.yml (2)
12-12: LGTM!The
flightctl_organizationaddition to credential defaults is consistent with other files in this PR.
46-46: API version updates are complete and consistent.All resource definitions and
api_versionparameters have been updated tov1beta1, maintaining consistency across Device and Repository resources.Also applies to: 61-61, 82-82, 102-102, 116-118, 186-186
plugins/module_utils/api_module.py (2)
121-142: Clean implementation for injecting organization ID into API requests.The wrapper pattern for injecting
org_idinto query parameters is well-structured. The guard clause on line 125 correctly skips injection when no organization is configured, and the check on line 137 prevents duplicateorg_idparameters. Mutatingquery_paramsin-place is safe because the OpenAPI-generatedApiClientcreates fresh parameter objects for each request, so in-place modifications do not affect subsequent calls.
427-427: Enum usage confirmed for flightctl SDK compatibility.The enum values
ConditionType.CertificateSigningRequestApproved,ConditionType.CertificateSigningRequestDenied, andConditionStatus.ConditionStatusTrueare correctly implemented according to the flightctl-python-client SDK and match the expected string values ("Approved", "Denied", "True") used in the integration tests.tests/integration/targets/flightctl_resource_info/tasks/fleets.yml (1)
8-8: LGTM!The addition of
flightctl_organizationto the connection_info follows the same optional parameter pattern asflightctl_token, usingdefault(omit)to ensure the parameter is completely omitted when not defined.tests/integration/targets/flightctl_resource/tasks/update-device.yml (2)
7-7: LGTM!The addition of
flightctl_organizationfollows the established pattern for optional connection parameters.
15-15: LGTM!The API version updates from
v1alpha1tov1beta1align with the PR objectives to support Flight Control API v1beta1.Also applies to: 29-29
tests/unit/plugins/module_utils/fixtures/client.yaml (1)
4-4: LGTM!The hardcoded nil UUID (
00000000-0000-0000-0000-000000000000) is appropriate for test fixtures.tests/integration/targets/inventory_flightctl/inventory_setup_test.yml (1)
12-12: LGTM!The organization field is correctly added to both the connection_info and the generated client.yaml configuration, maintaining consistency with the optional parameter pattern.
Also applies to: 79-79
tests/integration/targets/flightctl_resource_info/tasks/repository.yml (1)
8-8: LGTM!The organization field addition is consistent with the established pattern for optional connection parameters.
tests/integration/targets/flightctl_resource_info/tasks/resource-sync.yml (1)
8-8: LGTM!The organization field addition follows the established pattern for optional connection parameters.
tests/integration/targets/inventory_flightctl/inventory_test.yml (1)
100-100: LGTM!The organization field addition in the cleanup section is consistent with the established pattern for optional connection parameters.
Makefile (1)
24-29: LGTM! Configuration extraction updated correctly.The changes properly extract the new
access-tokenfield (instead oftoken) and add organization extraction from the client.yaml configuration file. The awk commands are correct for YAML key-value extraction.plugins/module_utils/config_loader.py (1)
49-56: LGTM! Schema correctly extended to support flexible authentication.The schema updates properly support the new token structure with
access-token,refresh-token, andtoken-to-usefields, along with the new top-levelorganizationfield. This aligns with the broader PR changes to support organization-scoped requests and flexible token authentication.tests/integration/targets/flightctl_resource_info/tasks/devices.yml (2)
11-11: LGTM! Organization support properly added to test connection info.The addition of
flightctl_organizationwithdefault(omit)correctly makes the organization parameter optional in tests while allowing it to be configured when needed. This aligns with the broader organization support across the codebase.
71-71: LGTM! API version consistently updated to v1beta1.The explicit
api_versionspecifications have been correctly updated toflightctl.io/v1beta1, aligning with the collection-wide API version migration. Note that other device creations in this file will use the module's default api_version, which provides good test coverage for both explicit and default API version usage.Also applies to: 82-82
plugins/module_utils/core.py (1)
26-28: LGTM! Organization parameter properly integrated.The
flightctl_organizationparameter has been correctly added following the established patterns:
- AUTH_ARGSPEC entry with environment fallback to
FLIGHTCTL_ORGANIZATION- short_params mapping for config file integration
- Module attribute with appropriate default value (None)
This enables organization to be specified via command-line arguments, environment variables, or configuration files, consistent with other authentication parameters.
Also applies to: 68-68, 78-78
plugins/inventory/flightctl.py (2)
45-48: LGTM! Documentation properly describes organization option.The new
organizationparameter is well-documented, following the same pattern as other configuration options in the inventory plugin.
280-282: LGTM! Organization properly integrated into inventory configuration.The organization parameter is correctly extracted with proper precedence (inventory option > config file) and conditionally assigned to the Configuration object. This implementation follows the established patterns for optional parameters in this plugin.
Based on learnings, the optional nature of organization aligns with the plugin's flexible authentication model.
Also applies to: 301-302
SiddarthR56
force-pushed
the
fac_1.0
branch
2 times, most recently
from
8224aae to
17f5a29
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/targets/flightctl_certificate_management/tasks/enrollment-denial.yml (1)
76-81: Fix incorrect variable reference in assertion.Line 80 references
check_resultbut should referenceenrollment_result(registered at line 74). The assertion is checking the enrollment request status before the denial instead of after.🔎 Proposed fix
- name: Assert that the enrollment request is now denied ansible.builtin.assert: that: - enrollment_result is success - - check_result.result.data[0].status.get('approval', None) == None + - enrollment_result.result.data[0].status.get('approval', None) == None
♻️ Duplicate comments (2)
plugins/modules/flightctl_resource.py (1)
27-27: API version default updated consistently.The default
api_versionhas been updated fromflightctl.io/v1alpha1toflightctl.io/v1beta1in both the documentation (line 27) and the argument specification (line 130). The examples at lines 69 and 75 are also aligned with this change.The Technical Preview status of v1beta1 was already flagged in a previous review.
changelogs/changelog.yaml (1)
118-118: Remove trailing whitespace.Line 118 contains trailing whitespace after the blank line, which should be removed for consistency and file cleanliness. This was also noted in the previous review.
🔎 Proposed fix
release_date: '2025-12-21' - +
🧹 Nitpick comments (2)
plugins/module_utils/config_loader.py (1)
110-121: Replacesetattrwith direct attribute assignment.As flagged by static analysis (B010), using
setattrwith constant attribute names provides no benefit over direct assignment and reduces readability.🔎 Proposed refactor
if token_to_use == "refresh" and refresh_token: - setattr(self, "token", refresh_token) + self.token = refresh_token elif token_to_use == "access" and access_token: - setattr(self, "token", access_token) + self.token = access_token elif access_token: - setattr(self, "token", access_token) + self.token = access_token # Assign organization if present in any supported location organization = ( config_data.get("organization") ) if organization: - setattr(self, "organization", organization) + self.organization = organizationchangelogs/changelog.yaml (1)
111-113: Consider adding more specific details to major_changes.The AI summary indicates this PR includes specific changes like upgrading from v1alpha1 to v1beta1 and adding organization-scoped request support. These details would be valuable to include in the
major_changessection for better changelog clarity.💡 Suggested enhancement
major_changes: - - Added support for new features in Flight Control API v1.0. - - Updated the Ansible collection to support Flight Control API version 1.0. + - Updated the Ansible collection to support Flight Control API version 1.0 (v1beta1). + - Upgraded default API versions from v1alpha1 to v1beta1. + - Added organization-scoped request support across modules, inventory plugin, and configuration.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
.github/workflows/integration-tests.yamlCHANGELOG.rstMakefileREADME.mdchangelogs/changelog.yamldemo/create.ymldemo/files/device.ymldemo/files/fleet.ymldemo/inventory/prepare-devices-and-fleets.ymlgalaxy.ymlplugins/doc_fragments/auth.pyplugins/inventory/flightctl.pyplugins/module_utils/api_module.pyplugins/module_utils/config_loader.pyplugins/module_utils/core.pyplugins/module_utils/resources.pyplugins/modules/flightctl_resource.pytests/integration/targets/connection_flightctl_console/runme.shtests/integration/targets/connection_flightctl_console/test_connection.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-denial.ymltests/integration/targets/flightctl_certificate_management/tasks/enrollment-approval.ymltests/integration/targets/flightctl_certificate_management/tasks/enrollment-denial.ymltests/integration/targets/flightctl_enrollment_config_info/tasks/fetch-config.ymltests/integration/targets/flightctl_resource/tasks/update-device.ymltests/integration/targets/flightctl_resource_info/tasks/devices.ymltests/integration/targets/flightctl_resource_info/tasks/fleets.ymltests/integration/targets/flightctl_resource_info/tasks/repository.ymltests/integration/targets/flightctl_resource_info/tasks/resource-sync.ymltests/integration/targets/inventory_flightctl/inventory_setup_test.ymltests/integration/targets/inventory_flightctl/inventory_test.ymltests/integration/targets/inventory_flightctl/runme.shtests/unit/plugins/module_utils/fixtures/client.yaml
🚧 Files skipped from review as they are similar to previous changes (18)
- tests/integration/targets/connection_flightctl_console/runme.sh
- tests/integration/targets/connection_flightctl_console/test_connection.yml
- tests/integration/targets/flightctl_resource_info/tasks/resource-sync.yml
- demo/inventory/prepare-devices-and-fleets.yml
- tests/integration/targets/flightctl_resource_info/tasks/repository.yml
- tests/integration/targets/inventory_flightctl/inventory_setup_test.yml
- tests/unit/plugins/module_utils/fixtures/client.yaml
- galaxy.yml
- plugins/module_utils/core.py
- tests/integration/targets/flightctl_enrollment_config_info/tasks/fetch-config.yml
- tests/integration/targets/flightctl_resource_info/tasks/devices.yml
- CHANGELOG.rst
- tests/integration/targets/flightctl_resource/tasks/update-device.yml
- .github/workflows/integration-tests.yaml
- demo/files/device.yml
- README.md
- demo/files/fleet.yml
- Makefile
🧰 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.
📚 Learning: 2025-04-24T17:24:38.607Z
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.
Applied to files:
tests/integration/targets/inventory_flightctl/runme.shtests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.ymltests/integration/targets/flightctl_certificate_management/tasks/enrollment-approval.ymltests/integration/targets/flightctl_certificate_management/tasks/enrollment-denial.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-denial.ymltests/integration/targets/inventory_flightctl/inventory_test.ymlplugins/modules/flightctl_resource.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:
tests/integration/targets/inventory_flightctl/runme.shtests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-denial.ymlplugins/inventory/flightctl.pydemo/create.ymlplugins/doc_fragments/auth.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:
tests/integration/targets/inventory_flightctl/runme.shplugins/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
🧬 Code graph analysis (2)
plugins/modules/flightctl_resource.py (1)
plugins/module_utils/resources.py (1)
api_version(28-29)
plugins/module_utils/config_loader.py (1)
plugins/module_utils/api_module.py (1)
get(178-212)
🪛 Ruff (0.14.8)
plugins/module_utils/config_loader.py
109-109: Possible hardcoded password assigned to: "token_to_use"
(S105)
110-110: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
111-111: Possible hardcoded password assigned to: "token_to_use"
(S105)
112-112: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
114-114: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
121-121: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
⏰ 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). (2)
- GitHub Check: test-unit-downstream
- GitHub Check: integration-tests (stable-2.16, 3.12)
🔇 Additional comments (21)
tests/integration/targets/flightctl_resource_info/tasks/fleets.yml (1)
8-8: LGTM! Organization parameter added correctly.The addition of
flightctl_organizationto the connection info follows the same pattern as other optional parameters and properly usesdefault(omit)to make it optional. This aligns with the PR's goal of adding organization-scoped request support.tests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-denial.yml (1)
8-8: LGTM! API version upgrade and organization scoping added.The addition of
flightctl_organizationwithdefault(omit)maintains backward compatibility while enabling organization-scoped requests. The API version upgrade tov1beta1aligns with the PR objectives.Also applies to: 16-16
tests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.yml (1)
8-8: LGTM! Consistent API version upgrade and organization scoping.The changes mirror those in the denial test file, maintaining consistency across the test suite.
Also applies to: 16-16
tests/integration/targets/flightctl_certificate_management/tasks/enrollment-denial.yml (1)
8-8: LGTM! API version upgrade and organization scoping added.The changes are consistent with the pattern established in the other test files.
Also applies to: 16-16
tests/integration/targets/flightctl_certificate_management/tasks/enrollment-approval.yml (1)
8-8: LGTM! API version upgrade and organization scoping added.The changes are consistent with the PR objectives and maintain the test's correctness.
Also applies to: 16-16
plugins/module_utils/resources.py (1)
43-43: LGTM! Python 3 modernization is correct.The replacement of
string_typeswith nativestris appropriate given that ansible-core 2.15 requires Python 3.9+. This eliminates thesixdependency and makes the code more idiomatic. Type hints and runtime checks are now consistent.Also applies to: 47-47
tests/integration/targets/inventory_flightctl/runme.sh (3)
12-12: LGTM - Optional field handled correctly!The
|| truefallback properly handles the case whereflightctl_organizationis missing fromintegration_config.yml, preventing script exit underset -e. This correctly implements the fix suggested in the previous review.
24-24: LGTM - Organization field added to inventory config.The organization field is correctly added to the generated inventory configuration, maintaining consistency with other connection parameters.
89-89: LGTM - Consistent parameter propagation.The organization parameter is correctly propagated to both test playbooks alongside the existing host and token parameters, enabling organization-scoped testing.
Also applies to: 96-96
tests/integration/targets/inventory_flightctl/inventory_test.yml (1)
100-100: LGTM - Organization parameter integrated correctly.The
flightctl_organizationfield is properly added to theconnection_infomapping using thedefault(omit)pattern, which is the correct Ansible idiom for optional parameters. This maintains consistency with howflightctl_tokenis handled and enables organization-scoped operations for the resource cleanup tasks.plugins/doc_fragments/auth.py (1)
33-37: New organization option documented correctly.The
flightctl_organizationoption follows the established pattern for other auth fragment options, with proper environment variable fallback (FLIGHTCTL_ORGANIZATION) and consistent documentation style.plugins/inventory/flightctl.py (3)
45-48: Organization option added to inventory configuration.The new
organizationoption follows the same pattern as existing options withdefault: nullandtype: str.
280-282: Organization resolution follows established pattern.The organization value resolution logic correctly prioritizes the inventory option over the config file value, consistent with how other parameters (host, token, username, password) are handled.
301-302: Verify thatflightctl.configuration.Configurationsupports theorganizationattribute.The code conditionally assigns
organizationto the Configuration object returned by_setup_connection_configuration(), similar torequest_timeoutandverify_ssl. Ensure the flightctl-python-client version in use supports this attribute, as a missing attribute would raise an AttributeError at runtime.demo/create.yml (2)
12-12: Organization credential added correctly.The
flightctl_organizationis properly added to the credential defaults anchor usingdefault(omit), matching the pattern of other optional credentials.
46-46: API version references updated consistently throughout demo.All
apiVersionfields in resource definitions andapi_versionparameters have been updated fromflightctl.io/v1alpha1toflightctl.io/v1beta1consistently across device, repository, and other resource operations.Also applies to: 61, 82, 102, 116, 118, 186
plugins/module_utils/config_loader.py (1)
49-55: Schema updated for new authentication fields.The schema now supports the new token structure with
organization,access-token,refresh-token, andtoken-to-usefields, enabling more flexible authentication configuration.plugins/module_utils/api_module.py (3)
121-142: Organization ID injection via monkey-patching.The approach of wrapping
param_serializeto injectorg_idas a query parameter is a pragmatic solution that ensures all API calls include the organization context when configured. The implementation correctly:
- Guards against missing organization
- Prevents duplicate
org_idparameters- Preserves original serialization behavior
443-450: CSR approval condition creation updated for v1beta1.The approval condition now uses
ConditionStatus.ConditionStatusTrueand the newCertificateSigningRequestApproved/CertificateSigningRequestDeniedtypes, which aligns with the v1beta1 API changes.
427-428: TheConditionType.CertificateSigningRequestApprovedandConditionType.CertificateSigningRequestDeniedenum values are imported from flightctl-python-client and actively used in the code at lines 427-428, 432, and 445. These enum values were made available following the merge of PR #1104 in the flightctl repository, which addressed CSR approval endpoint functionality. The integration tests that were previously pending this functionality are now active and passing, confirming availability of these enum values.changelogs/changelog.yaml (1)
99-108: LGTM: Formatting improvements for 1.3.0 release notes.The line wrapping and formatting adjustments improve readability without changing the actual content of the 1.3.0 release notes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
changelogs/changelog.yaml (1)
118-118: Remove trailing whitespace.Line 118 contains trailing whitespace. Please remove it for consistency with the rest of the file.
🔎 Proposed fix
release_date: '2025-12-21' -Remove the blank line or ensure it contains no trailing spaces.
Based on previous review feedback.
plugins/modules/flightctl_resource.py (1)
27-27: Note: v1beta1 API version is for Technical Preview.As previously noted, the v1beta1 API designation indicates beta status. Ensure users are aware this is Technical Preview and not recommended for production use.
🧹 Nitpick comments (2)
changelogs/changelog.yaml (1)
103-103: Optional: Remove trailing blank line.Line 103 appears to contain only whitespace. While this doesn't break YAML parsing, removing it would improve consistency with the rest of the file where blank lines separate major sections rather than appearing within a
changesblock.plugins/module_utils/config_loader.py (1)
111-124: Consider adding fallback logic for missing tokens.When
token-to-useis"refresh"butrefresh-tokenis missing (lines 113-116), the code warns but does not fall back toaccess-tokeneven if available. Similarly, when set to"access"but missing (lines 119-122), no fallback occurs.Consider adding fallback logic after warnings to use whatever token is available, improving resilience:
🔎 Proposed enhancement with fallback logic
if token_to_use == "refresh" and refresh_token: setattr(self, "token", refresh_token) elif token_to_use == "refresh" and not refresh_token: self._warn( "Config file: token-to-use is 'refresh' but refresh-token is missing; using access-token if present." ) + if access_token: + setattr(self, "token", access_token) elif token_to_use == "access" and access_token: setattr(self, "token", access_token) elif token_to_use == "access" and not access_token: self._warn( "Config file: token-to-use is 'access' but access-token is missing; no token will be set." ) + if refresh_token: + setattr(self, "token", refresh_token) elif access_token: setattr(self, "token", access_token) + elif refresh_token: + setattr(self, "token", refresh_token)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
.github/workflows/integration-tests.yamlCHANGELOG.rstMakefileREADME.mdchangelogs/changelog.yamldemo/create.ymldemo/files/device.ymldemo/files/fleet.ymldemo/inventory/prepare-devices-and-fleets.ymlgalaxy.ymlplugins/doc_fragments/auth.pyplugins/inventory/flightctl.pyplugins/module_utils/api_module.pyplugins/module_utils/config_loader.pyplugins/module_utils/core.pyplugins/module_utils/resources.pyplugins/modules/flightctl_resource.pytests/integration/targets/connection_flightctl_console/runme.shtests/integration/targets/connection_flightctl_console/test_connection.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-denial.ymltests/integration/targets/flightctl_certificate_management/tasks/enrollment-approval.ymltests/integration/targets/flightctl_certificate_management/tasks/enrollment-denial.ymltests/integration/targets/flightctl_enrollment_config_info/tasks/fetch-config.ymltests/integration/targets/flightctl_resource/tasks/update-device.ymltests/integration/targets/flightctl_resource_info/tasks/devices.ymltests/integration/targets/flightctl_resource_info/tasks/fleets.ymltests/integration/targets/flightctl_resource_info/tasks/repository.ymltests/integration/targets/flightctl_resource_info/tasks/resource-sync.ymltests/integration/targets/inventory_flightctl/inventory_setup_test.ymltests/integration/targets/inventory_flightctl/inventory_test.ymltests/integration/targets/inventory_flightctl/runme.shtests/unit/plugins/module_utils/fixtures/client.yaml
🚧 Files skipped from review as they are similar to previous changes (22)
- tests/integration/targets/flightctl_resource_info/tasks/fleets.yml
- tests/integration/targets/inventory_flightctl/runme.sh
- tests/integration/targets/connection_flightctl_console/test_connection.yml
- tests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-denial.yml
- tests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.yml
- tests/integration/targets/inventory_flightctl/inventory_setup_test.yml
- tests/integration/targets/flightctl_certificate_management/tasks/enrollment-denial.yml
- tests/integration/targets/flightctl_enrollment_config_info/tasks/fetch-config.yml
- galaxy.yml
- CHANGELOG.rst
- tests/integration/targets/connection_flightctl_console/runme.sh
- Makefile
- tests/integration/targets/flightctl_resource_info/tasks/resource-sync.yml
- tests/integration/targets/flightctl_resource/tasks/update-device.yml
- tests/integration/targets/flightctl_certificate_management/tasks/enrollment-approval.yml
- plugins/module_utils/resources.py
- plugins/doc_fragments/auth.py
- demo/files/fleet.yml
- .github/workflows/integration-tests.yaml
- tests/unit/plugins/module_utils/fixtures/client.yaml
- tests/integration/targets/inventory_flightctl/inventory_test.yml
- demo/create.yml
🧰 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.
📚 Learning: 2025-04-24T17:24:38.607Z
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.
Applied to files:
tests/integration/targets/flightctl_resource_info/tasks/devices.ymltests/integration/targets/flightctl_resource_info/tasks/repository.ymlplugins/modules/flightctl_resource.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/module_utils/core.pydemo/inventory/prepare-devices-and-fleets.ymlplugins/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
📚 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
🧬 Code graph analysis (3)
plugins/module_utils/core.py (1)
plugins/module_utils/config_loader.py (1)
ConfigLoader(26-154)
plugins/module_utils/config_loader.py (1)
plugins/module_utils/api_module.py (1)
get(178-212)
plugins/modules/flightctl_resource.py (1)
plugins/module_utils/resources.py (1)
api_version(29-30)
🪛 Ruff (0.14.10)
plugins/module_utils/config_loader.py
111-111: Possible hardcoded password assigned to: "token_to_use"
(S105)
112-112: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
113-113: Possible hardcoded password assigned to: "token_to_use"
(S105)
117-117: Possible hardcoded password assigned to: "token_to_use"
(S105)
118-118: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
119-119: Possible hardcoded password assigned to: "token_to_use"
(S105)
124-124: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
131-131: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
⏰ 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-sanity-downstream
- GitHub Check: test-unit-upstream (stable-2.16, 3.12)
- GitHub Check: integration-tests (stable-2.16, 3.12)
🔇 Additional comments (16)
changelogs/changelog.yaml (1)
117-117: Verify the release date.The
release_dateis set to'2025-12-21', but this PR was created on 2025-12-22. Confirm whether backdating by one day is intentional or if this should be updated to'2025-12-22'.README.md (1)
96-101: LGTM! Clear example of organization scoping.The new example effectively demonstrates how to use the
flightctl_organizationparameter to scope device creation to a specific organization.demo/inventory/prepare-devices-and-fleets.yml (2)
14-14: LGTM! Organization support added to credential defaults.The addition of
flightctl_organizationto the shared credential defaults properly enables organization-scoped operations throughout the playbook.
27-27: LGTM! Consistent API version migration to v1beta1.All Device and Fleet resource definitions have been updated to use
flightctl.io/v1beta1, ensuring consistency across the demo configuration.Also applies to: 48-48, 69-69, 91-91, 111-111, 131-131, 150-150, 202-202
plugins/module_utils/config_loader.py (2)
27-27: LGTM! Warn callback mechanism properly implemented.The addition of
warn_callbackparameter and the_warnmethod enables proper surfacing of configuration issues to calling modules.Also applies to: 148-150
127-131: LGTM! Organization field properly extracted and assigned.The organization field is correctly parsed from the configuration and assigned to the instance for propagation to API calls.
demo/files/device.yml (1)
2-2: LGTM! Device manifest updated to v1beta1.The API version has been properly updated to align with the v1.0 API migration.
tests/integration/targets/flightctl_resource_info/tasks/repository.yml (1)
8-8: LGTM! Organization field added to test configuration.The
flightctl_organizationfield is properly added to the connection_info, enabling organization-scoped testing when the parameter is provided.plugins/inventory/flightctl.py (2)
45-48: LGTM! Organization option properly documented.The new
organizationoption is clearly documented with appropriate description and type information.
280-282: LGTM! Organization handling properly implemented.The organization value is correctly extracted from inventory options with fallback to config file, and properly assigned to the Configuration object when present. This follows the established pattern for other configuration parameters.
Also applies to: 301-302
tests/integration/targets/flightctl_resource_info/tasks/devices.yml (2)
11-11: LGTM! Organization field added to test configuration.The
flightctl_organizationfield is properly added to enable organization-scoped testing.
71-71: LGTM! API version updated to v1beta1.The
api_versionparameters are correctly updated toflightctl.io/v1beta1, aligning with the collection's migration to v1.0 APIs.Also applies to: 82-82
plugins/module_utils/core.py (3)
15-15: LGTM: Appropriate removal of Python 2 compatibility layer.The direct import from
urllib.parseis correct for ansible-core 2.15.0, which requires Python 3.9+. Removing thesixcompatibility layer is a good cleanup.
26-28: LGTM: Organization parameter properly integrated.The new
flightctl_organizationparameter follows the established pattern for authentication parameters:
- Environment fallback via
FLIGHTCTL_ORGANIZATION- Proper mapping in
short_params- Declared as an optional attribute
The implementation is consistent with how other configuration parameters are handled.
Also applies to: 68-68, 78-78
154-154: LGTM: Proper warning callback integration.Passing
warn_callback=self.warnallowsConfigLoaderto emit warnings through the module's warning system, ensuring proper integration with Ansible's warning infrastructure.plugins/module_utils/api_module.py (1)
427-445: Verify these API constants exist in the flightctl-python-client v1.0 dependency.The code uses
ConditionType.CertificateSigningRequestApproved,ConditionType.CertificateSigningRequestDenied, andConditionStatus.ConditionStatusTruefor CSR condition handling. Confirm these constants are available in the version of flightctl-python-client specified in the project dependencies, as they appear to be part of the v1.0 API update.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
plugins/module_utils/api_module.py (1)
119-142: Implementation is correct; unit test gap was flagged in prior review.The monkey-patching approach properly:
- Guards against missing
organization- Avoids duplicate
org_identries- Preserves original serialization behavior
The missing unit test coverage concern was already raised in a previous review.
🧹 Nitpick comments (1)
plugins/module_utils/config_loader.py (1)
127-131: LGTM: Organization parsing and warning mechanism implemented correctly.The organization extraction and assignment logic is straightforward, and the
_warnhelper properly guards the callback invocation.Note: The static analysis tool flags
setattrusage (B010), suggesting direct assignment (self.organization = organization). While this is a valid suggestion for consistency, the current approach is acceptable given the dynamic attribute assignment pattern used throughout this file.Also applies to: 148-150
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
.github/workflows/integration-tests.yamlCHANGELOG.rstMakefileREADME.mdchangelogs/changelog.yamldemo/create.ymldemo/files/device.ymldemo/files/fleet.ymldemo/inventory/prepare-devices-and-fleets.ymlgalaxy.ymlplugins/doc_fragments/auth.pyplugins/inventory/flightctl.pyplugins/module_utils/api_module.pyplugins/module_utils/config_loader.pyplugins/module_utils/core.pyplugins/module_utils/resources.pyplugins/modules/flightctl_resource.pytests/integration/targets/connection_flightctl_console/runme.shtests/integration/targets/connection_flightctl_console/test_connection.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-denial.ymltests/integration/targets/flightctl_certificate_management/tasks/enrollment-approval.ymltests/integration/targets/flightctl_certificate_management/tasks/enrollment-denial.ymltests/integration/targets/flightctl_enrollment_config_info/tasks/fetch-config.ymltests/integration/targets/flightctl_resource/tasks/update-device.ymltests/integration/targets/flightctl_resource_info/tasks/devices.ymltests/integration/targets/flightctl_resource_info/tasks/fleets.ymltests/integration/targets/flightctl_resource_info/tasks/repository.ymltests/integration/targets/flightctl_resource_info/tasks/resource-sync.ymltests/integration/targets/inventory_flightctl/inventory_setup_test.ymltests/integration/targets/inventory_flightctl/inventory_test.ymltests/integration/targets/inventory_flightctl/runme.shtests/unit/plugins/module_utils/fixtures/client.yaml
🚧 Files skipped from review as they are similar to previous changes (15)
- tests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-denial.yml
- tests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.yml
- tests/integration/targets/inventory_flightctl/runme.sh
- demo/create.yml
- tests/integration/targets/flightctl_resource_info/tasks/repository.yml
- tests/integration/targets/flightctl_resource_info/tasks/fleets.yml
- tests/integration/targets/flightctl_enrollment_config_info/tasks/fetch-config.yml
- plugins/inventory/flightctl.py
- tests/integration/targets/flightctl_resource_info/tasks/resource-sync.yml
- tests/integration/targets/flightctl_resource/tasks/update-device.yml
- tests/integration/targets/flightctl_resource_info/tasks/devices.yml
- tests/integration/targets/inventory_flightctl/inventory_test.yml
- plugins/module_utils/core.py
- demo/files/device.yml
- galaxy.yml
🧰 Additional context used
🧠 Learnings (4)
📓 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.
📚 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:
tests/integration/targets/inventory_flightctl/inventory_setup_test.ymldemo/inventory/prepare-devices-and-fleets.ymltests/integration/targets/connection_flightctl_console/test_connection.ymltests/unit/plugins/module_utils/fixtures/client.yamlplugins/doc_fragments/auth.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:
tests/integration/targets/inventory_flightctl/inventory_setup_test.ymltests/unit/plugins/module_utils/fixtures/client.yaml
📚 Learning: 2025-04-24T17:24:38.607Z
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.
Applied to files:
tests/integration/targets/flightctl_certificate_management/tasks/enrollment-approval.ymltests/integration/targets/connection_flightctl_console/runme.shchangelogs/changelog.yamltests/integration/targets/flightctl_certificate_management/tasks/enrollment-denial.ymlplugins/modules/flightctl_resource.py.github/workflows/integration-tests.yaml
🧬 Code graph analysis (1)
plugins/modules/flightctl_resource.py (1)
plugins/module_utils/resources.py (1)
api_version(29-30)
🪛 Ruff (0.14.10)
plugins/module_utils/config_loader.py
111-111: Possible hardcoded password assigned to: "token_to_use"
(S105)
112-112: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
113-113: Possible hardcoded password assigned to: "token_to_use"
(S105)
117-117: Possible hardcoded password assigned to: "token_to_use"
(S105)
118-118: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
119-119: Possible hardcoded password assigned to: "token_to_use"
(S105)
124-124: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
131-131: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
⏰ 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 (21)
.github/workflows/integration-tests.yaml (2)
88-96: LGTM!The disk space freeing steps are consistently duplicated in the downstream job, which is appropriate since both jobs deploy flightctl services and would benefit from the freed space.
37-44: Good approach to freeing disk space before deployment.The two-step process effectively addresses runner space constraints: the GitHub action handles systematic cleanup of packages and tools, and the
podman system prunewith|| trueensures podman-specific cleanup without failing the workflow if no images exist.demo/files/fleet.yml (1)
2-2: LGTM! API version updated correctly.The apiVersion bump from
v1alpha1tov1beta1aligns with the PR's goal to update to Flight Control API v1.0. The Fleet manifest structure remains unchanged.tests/integration/targets/flightctl_certificate_management/tasks/enrollment-approval.yml (1)
8-8: LGTM! Organization scoping and API version updates are correct.The addition of
flightctl_organizationto connection_info and the API version bump tov1beta1are consistent with the PR's objectives to support Flight Control API v1.0 with organization scoping. Thedefault(omit)pattern properly handles the optional organization parameter.Also applies to: 16-16
CHANGELOG.rst (1)
7-20: LGTM! Changelog properly documents the v1.4.0 release.The new v1.4.0 section correctly documents the addition of Flight Control API v1.0 support. The structure follows the established changelog format, and the major changes clearly communicate the scope of this update.
tests/integration/targets/connection_flightctl_console/test_connection.yml (1)
11-11: LGTM! Organization parameter added correctly.The
ansible_flightctl_organizationvariable is properly integrated into the connection plugin test configuration, following the same pattern as other FlightCtl connection parameters with thedefault(omit)pattern for optional handling.tests/integration/targets/flightctl_certificate_management/tasks/enrollment-denial.yml (1)
8-8: LGTM! Consistent with enrollment-approval.yml changes.The addition of
flightctl_organizationand the API version bump tov1beta1match the identical changes in enrollment-approval.yml, ensuring consistent organization scoping across certificate management tests.Also applies to: 16-16
changelogs/changelog.yaml (1)
109-118: LGTM! Version 1.4.0 changelog entry is properly structured.The new release entry follows the established format with correct YAML indentation and structure. The major changes and release summary accurately reflect the Flight Control API v1.0 support.
demo/inventory/prepare-devices-and-fleets.yml (2)
14-14: LGTM! Organization parameter added to credentials.The
flightctl_organizationfield is properly integrated into thecredential_defaultsanchor, allowing it to propagate to all resource creation tasks via the alias pattern.
27-27: LGTM! All resource apiVersions consistently updated to v1beta1.All Device and Fleet resources have been correctly updated from
flightctl.io/v1alpha1toflightctl.io/v1beta1, maintaining consistency across the demo inventory setup. The resource specifications remain unchanged aside from the version bump.Also applies to: 48-48, 69-69, 91-91, 111-111, 131-131, 150-150, 202-202
Makefile (1)
24-29: Configuration extraction updated for Flight Control API client.yaml format.The Makefile changes correctly extract the
access-token,server, andorganizationfields from the client configuration file and propagate them tointegration_config.yml. All field names are validated against the Flight Control configuration schema. The shell syntax and line continuations are correct.plugins/module_utils/config_loader.py (2)
27-27: LGTM: Clean warn callback integration.The addition of the
warn_callbackparameter and its storage as a private attribute follows good encapsulation practices.Also applies to: 33-33
51-51: LGTM: Schema properly extended for organization and token selection.The schema updates correctly add organization and the new token fields as optional strings, maintaining backward compatibility.
Also applies to: 55-57
plugins/module_utils/resources.py (1)
44-44: LGTM: Correct removal of Python 2 compatibility code.The change from
string_typesto built-instris appropriate given the collection's requirement for Python 3.10+. This simplifies the code and removes the dependency onsix.Also applies to: 48-48
plugins/doc_fragments/auth.py (1)
33-37: LGTM: Organization parameter properly documented.The documentation for
flightctl_organizationis clear, follows the existing pattern for other authentication parameters, and correctly documents the environment variable fallback.tests/integration/targets/inventory_flightctl/inventory_setup_test.yml (1)
12-12: LGTM: Organization added to connection_info.The
flightctl_organizationfield is properly added to the connection info withdefault(omit)to handle cases where it's not provided.tests/integration/targets/connection_flightctl_console/runme.sh (1)
12-12: LGTM: Organization parsing and propagation implemented correctly.The script properly extracts
flightctl_organizationfrom the integration config and passes it to the playbook as an extra variable, following the same pattern as the existingflightctl_hostandflightctl_tokenparameters.Also applies to: 14-18
plugins/modules/flightctl_resource.py (1)
27-27: LGTM: API version consistently updated to v1beta1.The default
api_versionhas been updated fromflightctl.io/v1alpha1toflightctl.io/v1beta1consistently across documentation, examples, and the argument specification. This aligns with the PR objectives to update to v1.0 APIs.Note: Users relying on the previous default will need to explicitly specify
api_version: flightctl.io/v1alpha1if they need to continue using the alpha API.Also applies to: 69-69, 75-75, 130-130
README.md (2)
59-59: LGTM: Version appropriately bumped to 1.4.0.The version update to 1.4.0 is appropriate given the addition of organization support and the API version change to v1beta1.
Also applies to: 65-65
88-88: LGTM: Examples updated consistently with new features.The examples have been properly updated to:
- Use
api_version: flightctl.io/v1beta1throughout- Demonstrate the new
flightctl_organizationparameter with a clear exampleThe new organization example (lines 96-101) effectively shows users how to scope requests to a specific organization.
Also applies to: 96-101, 107-109
plugins/module_utils/api_module.py (1)
427-445: Enum updates align with v1beta1 API schema.The condition type and status enum values are correctly updated:
ConditionType.CertificateSigningRequestApproved/CertificateSigningRequestDeniedConditionStatus.ConditionStatusTrueThe idempotency logic correctly filters existing approval/denial conditions, removes conflicting states when needed, and adds the new condition with proper metadata (timestamp, reason, message).
|
|
||
| env: | ||
| FLIGHTCTL_REF: v0.10.0 | ||
| FLIGHTCTL_REF: v1.0.0 |
There was a problem hiding this comment.
The FLIGHTCTL_REF tag v1.0.0 does not exist in the flightctl/flightctl repository.
The latest available release is v0.8.1. Update FLIGHTCTL_REF to an existing tag or confirm the correct version that should be referenced before this workflow will function properly.
🤖 Prompt for AI Agents
In .github/workflows/integration-tests.yaml around line 14, the FLIGHTCTL_REF
value is set to a non-existent tag "v1.0.0"; update this to an existing release
(for example "v0.8.1") or replace it with the correct tag/branch you intend to
use, then commit the change so the workflow references a valid
flightctl/flightctl tag.
tests/integration/targets/inventory_flightctl/inventory_setup_test.yml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugins/module_utils/api_module.py (1)
119-142: Organization query parameter injection is correctly implemented.The monkey-patching approach properly:
- Guards against missing organization configuration
- Prevents duplicate
org_identries in query parameters- Preserves original serialization behavior
However, as noted in a previous review, this feature lacks unit test coverage. The previous reviewer requested tests to verify:
- Correct
org_idinjection when organization is set- No duplicate
org_identries- Early return when organization is not set
- Preservation of original
param_serializebehavior
🧹 Nitpick comments (1)
tests/integration/targets/connection_flightctl_console/runme.sh (1)
14-18: LGTM! Multiline format improves readability.The refactored multiline format makes the
ansible-playbookinvocation more readable, and the newflightctl_organizationparameter is correctly passed as an extra variable with proper quoting.Optional: Add validation for required configuration values
Consider adding validation to ensure all required values are present before invoking ansible-playbook:
FLIGHTCTL_HOST=$(grep -oP 'flightctl_host:\s*\K.*' "../../integration_config.yml") FLIGHTCTL_TOKEN=$(grep -oP 'flightctl_token:\s*\K.*' "../../integration_config.yml") FLIGHTCTL_ORGANIZATION=$(grep -oP 'flightctl_organization:\s*\K.*' "../../integration_config.yml") + +# Validate required configuration values +if [[ -z "${FLIGHTCTL_HOST}" ]] || [[ -z "${FLIGHTCTL_TOKEN}" ]] || [[ -z "${FLIGHTCTL_ORGANIZATION}" ]]; then + echo "Error: Missing required configuration values in integration_config.yml" + exit 1 +fi ansible-playbook "./test_connection.yml" \This would provide clearer error messages if configuration values are missing.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
.github/workflows/integration-tests.yamlCHANGELOG.rstMakefileREADME.mdchangelogs/changelog.yamldemo/create.ymldemo/files/device.ymldemo/files/fleet.ymldemo/inventory/prepare-devices-and-fleets.ymlgalaxy.ymlplugins/doc_fragments/auth.pyplugins/inventory/flightctl.pyplugins/module_utils/api_module.pyplugins/module_utils/config_loader.pyplugins/module_utils/core.pyplugins/module_utils/resources.pyplugins/modules/flightctl_resource.pytests/integration/targets/connection_flightctl_console/runme.shtests/integration/targets/connection_flightctl_console/test_connection.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-denial.ymltests/integration/targets/flightctl_certificate_management/tasks/enrollment-approval.ymltests/integration/targets/flightctl_certificate_management/tasks/enrollment-denial.ymltests/integration/targets/flightctl_enrollment_config_info/tasks/fetch-config.ymltests/integration/targets/flightctl_resource/tasks/update-device.ymltests/integration/targets/flightctl_resource_info/tasks/devices.ymltests/integration/targets/flightctl_resource_info/tasks/fleets.ymltests/integration/targets/flightctl_resource_info/tasks/repository.ymltests/integration/targets/flightctl_resource_info/tasks/resource-sync.ymltests/integration/targets/inventory_flightctl/inventory_setup_test.ymltests/integration/targets/inventory_flightctl/inventory_test.ymltests/integration/targets/inventory_flightctl/runme.shtests/unit/plugins/module_utils/fixtures/client.yaml
🚧 Files skipped from review as they are similar to previous changes (20)
- tests/integration/targets/inventory_flightctl/inventory_setup_test.yml
- tests/integration/targets/flightctl_resource_info/tasks/fleets.yml
- tests/integration/targets/inventory_flightctl/runme.sh
- tests/integration/targets/flightctl_resource/tasks/update-device.yml
- plugins/doc_fragments/auth.py
- Makefile
- tests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-denial.yml
- changelogs/changelog.yaml
- galaxy.yml
- plugins/inventory/flightctl.py
- tests/integration/targets/connection_flightctl_console/test_connection.yml
- demo/files/fleet.yml
- tests/integration/targets/flightctl_enrollment_config_info/tasks/fetch-config.yml
- demo/inventory/prepare-devices-and-fleets.yml
- tests/integration/targets/flightctl_certificate_management/tasks/enrollment-approval.yml
- tests/integration/targets/flightctl_certificate_management/tasks/enrollment-denial.yml
- tests/unit/plugins/module_utils/fixtures/client.yaml
- tests/integration/targets/flightctl_resource_info/tasks/resource-sync.yml
- plugins/modules/flightctl_resource.py
- .github/workflows/integration-tests.yaml
🧰 Additional context used
🧠 Learnings (3)
📓 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.
📚 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/module_utils/core.pydemo/create.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.yml
📚 Learning: 2025-04-24T17:24:38.607Z
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.
Applied to files:
tests/integration/targets/flightctl_resource_info/tasks/devices.ymltests/integration/targets/connection_flightctl_console/runme.shtests/integration/targets/flightctl_resource_info/tasks/repository.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.yml
🧬 Code graph analysis (1)
plugins/module_utils/core.py (1)
plugins/module_utils/config_loader.py (1)
ConfigLoader(26-160)
🪛 Ruff (0.14.10)
plugins/module_utils/config_loader.py
111-111: Possible hardcoded password assigned to: "token_to_use"
(S105)
113-113: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
115-115: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
120-120: Possible hardcoded password assigned to: "token_to_use"
(S105)
122-122: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
124-124: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
130-130: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
137-137: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
⏰ 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 (26)
tests/integration/targets/flightctl_resource_info/tasks/repository.yml (1)
8-8: LGTM! Organization support added consistently.The addition of
flightctl_organizationfollows the same optional pattern asflightctl_tokenand will be properly propagated through all resource operations via the YAML anchor. Thedefault(omit)ensures backwards compatibility.tests/integration/targets/inventory_flightctl/inventory_test.yml (1)
100-100: LGTM! Clean addition of organization support.The
flightctl_organizationfield follows the same pattern asflightctl_tokenand usesdefault(omit)to maintain backward compatibility. This ensures the parameter is only passed when explicitly set, preventing any breaking changes to existing test runs.tests/integration/targets/connection_flightctl_console/runme.sh (1)
12-12: LGTM! Consistent pattern for parsing organization.The parsing of
flightctl_organizationfollows the same pattern as the existingflightctl_hostandflightctl_tokenextraction, maintaining consistency in the script.demo/files/device.yml (1)
2-2: LGTM! API version update is correct.The apiVersion migration to
flightctl.io/v1beta1aligns with the PR's objective to support Flight Control API v1.0.CHANGELOG.rst (1)
7-20: LGTM! Release notes properly document v1.0 API support.The v1.4.0 release section clearly describes the addition of Flight Control API v1.0 support, consistent with the PR's objectives.
plugins/module_utils/resources.py (1)
44-48: LGTM! Proper modernization to use native Python 3 types.Replacing
string_typesfromsixwith nativestris appropriate given the collection requires Python 3.10+. This simplifies the code while maintaining identical behavior.demo/create.yml (2)
12-12: LGTM! Organization field properly added to credential defaults.The
flightctl_organizationfield is correctly added withomitfor optional usage, consistent with other credential parameters.
46-186: LGTM! API version migration is comprehensive and consistent.All resource definitions and api_version parameters have been consistently updated from
flightctl.io/v1alpha1toflightctl.io/v1beta1, aligning with the Flight Control API v1.0 support.tests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.yml (3)
8-8: LGTM! Organization field properly added.The
flightctl_organizationfield is correctly added toconnection_infowith proper optional handling usingomit.
16-16: LGTM! API version correctly updated.The api_version has been properly updated to
flightctl.io/v1beta1.
48-50: Verify that the upstream CSR approval issue has been resolved.Based on learnings, these assertions were previously commented out pending the merge of flightctl/flightctl#1104, which addressed issues with CSR approval endpoints. Since the assertions are now active, please confirm that PR #1104 has been merged and the approval endpoint issues are resolved.
Based on learnings, the CSR approval endpoint issues need to be resolved before these assertions can be safely enabled.
tests/integration/targets/flightctl_resource_info/tasks/devices.yml (2)
11-11: LGTM! Organization field properly added.The
flightctl_organizationfield is correctly added toconnection_infowith appropriate optional handling.
71-82: LGTM! API version updates are consistent.Both device resources have been properly updated to
api_version: flightctl.io/v1beta1.README.md (2)
59-65: LGTM! Version correctly bumped to 1.4.0.The collection version has been properly updated in both the requirements.yml example and the ansible-galaxy install command.
88-114: LGTM! Documentation comprehensively updated with v1beta1 and organization support.The usage examples properly demonstrate:
- API version migration to
flightctl.io/v1beta1- New organization-scoped device creation feature
- Consistent application of the new API version across all examples
The new organization example clearly shows users how to work with organization-scoped resources.
plugins/module_utils/api_module.py (1)
427-445: Verify enum constant names against the flightctl-python-client library version in use.The code imports and uses
ConditionTypeandConditionStatusenums fromflightctl.models(lines 30-31, 427, 432, 445). While the imports are correctly structured and the enum constants are actively used in the approval logic, the actual enum member names (CertificateSigningRequestApproved,CertificateSigningRequestDenied,ConditionStatusTrue) should be verified against the corresponding flightctl-python-client library to ensure they match the API's current enum definitions, particularly given ongoing changes referenced in flightctl PR #1104.plugins/module_utils/core.py (5)
15-15: LGTM: Import modernization.Removing the
sixlibrary dependency in favor of the standard libraryurllib.parseis appropriate for Python 3-only code.
26-28: LGTM: Organization parameter properly defined.The
flightctl_organizationparameter follows the established pattern with appropriate environment variable fallback.
68-68: LGTM: Organization mapping is consistent.The short_params mapping properly connects the
organizationattribute to theflightctl_organizationparameter for config file loading.
78-78: LGTM: Attribute declaration is correct.The
organizationattribute is properly typed and initialized with an appropriate default value.
154-154: LGTM: Warning callback properly wired.Passing
warn_callback=self.warnenables configuration warnings fromConfigLoaderto be surfaced through the module's warning system.plugins/module_utils/config_loader.py (5)
27-27: LGTM: Warning callback support added.The constructor now accepts an optional
warn_callbackparameter, enabling configuration warnings to be properly surfaced to the calling code.Also applies to: 33-33
51-51: LGTM: Schema updated for organization and granular token control.The schema now supports:
- Organization at the root level
- Separate
access-token,refresh-token, andtoken-to-usefields for more flexible authentication configurationAlso applies to: 55-57
105-130: LGTM: Token selection logic now implements proper fallbacks.The refactored token selection correctly implements fallback behavior:
- When
token-to-useis"refresh", it usesrefresh-tokenif available, falls back toaccess-token, and only warns if neither exists- When
token-to-useis"access", it usesaccess-tokenif available, falls back torefresh-token, and only warns if neither exists- When
token-to-useis not specified, it defaults toaccess-tokenif availableThis addresses the incomplete fallback logic flagged in the previous review.
133-137: LGTM: Organization parsing is correct.The organization field is properly extracted from the root configuration level and assigned when present, aligning with the schema and usage in
core.py.
154-156: LGTM: Warning method is properly defensive.The
_warnmethod safely checks if the callback is callable before invoking it, ensuring graceful handling when no callback is provided.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
changelogs/changelog.yaml (1)
118-118: Consider removing trailing whitespace.Line 118 appears to be a blank line that may contain trailing whitespace. While this doesn't affect YAML parsing, removing trailing spaces maintains consistency with coding standards.
Note: A similar issue on this line was previously flagged and marked as addressed in commit a5d36d0. If the trailing whitespace has been reintroduced, please remove it.
plugins/module_utils/api_module.py (1)
120-143: LGTM - Organization query parameter injection is well-implemented.The monkey-patching approach correctly:
- Guards against missing organization configuration (early return)
- Prevents duplicate
org_identries- Preserves original serialization behavior
A past review comment already flagged the need for unit tests, which remains valid.
.github/workflows/integration-tests.yaml (1)
14-14: Verify that the v1.0.0 tag exists in flightctl/flightctl repository.A previous review flagged that v1.0.0 doesn't exist in the flightctl/flightctl repository. Confirm that this tag has been created; otherwise, both workflow jobs will fail during checkout.
Run the following script to verify the tag exists:
#!/bin/bash # Description: Verify v1.0.0 tag exists in flightctl/flightctl repository # Check if v1.0.0 tag exists gh api repos/flightctl/flightctl/git/ref/tags/v1.0.0 --jq '.ref' || echo "Tag v1.0.0 not found" # Also list recent tags for reference echo "Recent tags in flightctl/flightctl:" gh api repos/flightctl/flightctl/tags --jq '.[0:5] | .[] | .name'
🧹 Nitpick comments (3)
CHANGELOG.rst (1)
13-13: Expand release notes with specific features and improvements.The release summary and major changes are generic and partially redundant. Both lines 18 and 19 essentially duplicate the same message ("Added support" + "Updated... to support"). Given the scope of changes described in the PR (organization-scoped requests, wiring across modules, config loader, inventory plugin updates), the release notes should be more specific and detailed.
🔎 Suggested improvements for release notes
Release Summary --------------- -Added support for Flight Control API v1.0. +Added support for Flight Control API v1.0, including organization-scoped request support and inventory plugin enhancements. Major Changes ------------- -- Added support for new features in Flight Control API v1.0. -- Updated the Ansible collection to support Flight Control API version 1.0. +- Added organization-scoped request support across modules and configuration loading. +- Updated default apiVersion to flightctl.io/v1beta1. +- Enhanced inventory plugin with improved organization-scoped device queries. +- Improved token-selection logic for authentication workflows.Also applies to: 18-19
changelogs/changelog.yaml (1)
99-105: LGTM: Formatting improvements to 1.3.0 release notes.The line wrapping of the longer changelog entries improves readability while maintaining valid YAML structure. The continuation lines are properly indented.
However, line 103 appears to be a blank line that may contain trailing whitespace. Consider removing any trailing spaces for consistency.
.github/workflows/integration-tests.yaml (1)
37-44: Update jlumbroso/free-disk-space to use the main branch instead of a version tag.The upstream project for
jlumbroso/free-disk-spacedoes not publish formal releases and recommends usingjlumbroso/free-disk-space@main. Replacev1.3.1with@mainto align with the project's recommended usage pattern and ensure you receive updates directly from the source.Also applies to: 88-95
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
.github/workflows/integration-tests.yamlCHANGELOG.rstMakefileREADME.mdchangelogs/changelog.yamldemo/create.ymldemo/files/device.ymldemo/files/fleet.ymldemo/inventory/prepare-devices-and-fleets.ymlgalaxy.ymlplugins/doc_fragments/auth.pyplugins/inventory/flightctl.pyplugins/module_utils/api_module.pyplugins/module_utils/config_loader.pyplugins/module_utils/core.pyplugins/module_utils/resources.pyplugins/modules/flightctl_resource.pytests/integration/targets/connection_flightctl_console/runme.shtests/integration/targets/connection_flightctl_console/test_connection.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.ymltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-denial.ymltests/integration/targets/flightctl_certificate_management/tasks/enrollment-approval.ymltests/integration/targets/flightctl_certificate_management/tasks/enrollment-denial.ymltests/integration/targets/flightctl_enrollment_config_info/tasks/fetch-config.ymltests/integration/targets/flightctl_resource/tasks/update-device.ymltests/integration/targets/flightctl_resource_info/tasks/devices.ymltests/integration/targets/flightctl_resource_info/tasks/fleets.ymltests/integration/targets/flightctl_resource_info/tasks/repository.ymltests/integration/targets/flightctl_resource_info/tasks/resource-sync.ymltests/integration/targets/inventory_flightctl/inventory_setup_test.ymltests/integration/targets/inventory_flightctl/inventory_test.ymltests/integration/targets/inventory_flightctl/runme.shtests/unit/plugins/module_utils/fixtures/client.yaml
🚧 Files skipped from review as they are similar to previous changes (20)
- Makefile
- tests/integration/targets/flightctl_certificate_management/tasks/enrollment-approval.yml
- tests/integration/targets/flightctl_resource/tasks/update-device.yml
- galaxy.yml
- tests/integration/targets/flightctl_resource_info/tasks/repository.yml
- plugins/module_utils/resources.py
- tests/integration/targets/flightctl_enrollment_config_info/tasks/fetch-config.yml
- tests/integration/targets/connection_flightctl_console/test_connection.yml
- tests/integration/targets/flightctl_resource_info/tasks/resource-sync.yml
- tests/integration/targets/inventory_flightctl/runme.sh
- tests/integration/targets/flightctl_resource_info/tasks/fleets.yml
- tests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-approval.yml
- demo/inventory/prepare-devices-and-fleets.yml
- tests/integration/targets/inventory_flightctl/inventory_test.yml
- plugins/module_utils/core.py
- plugins/inventory/flightctl.py
- tests/integration/targets/connection_flightctl_console/runme.sh
- demo/files/fleet.yml
- tests/integration/targets/inventory_flightctl/inventory_setup_test.yml
- demo/files/device.yml
🧰 Additional context used
🧠 Learnings (3)
📓 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.
📚 Learning: 2025-04-24T17:24:38.607Z
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.
Applied to files:
tests/integration/targets/flightctl_certificate_management/tasks/enrollment-denial.yml.github/workflows/integration-tests.yamltests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-denial.ymlchangelogs/changelog.yamltests/integration/targets/flightctl_resource_info/tasks/devices.ymlplugins/modules/flightctl_resource.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:
tests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-denial.ymldemo/create.ymlplugins/doc_fragments/auth.pytests/unit/plugins/module_utils/fixtures/client.yaml
🧬 Code graph analysis (1)
plugins/modules/flightctl_resource.py (1)
plugins/module_utils/resources.py (1)
api_version(29-30)
🪛 Ruff (0.14.10)
plugins/module_utils/config_loader.py
111-111: Possible hardcoded password assigned to: "token_to_use"
(S105)
113-113: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
115-115: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
120-120: Possible hardcoded password assigned to: "token_to_use"
(S105)
122-122: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
124-124: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
130-130: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
137-137: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
⏰ 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 (22)
CHANGELOG.rst (1)
7-8: Reconsider version bump to 2.0 for API v1.0 support.The past reviewer raised this concern, and it remains relevant. The PR title indicates this is an update to v1.0 APIs (a major API version), and the AI summary shows significant changes including organization-scoped requests wired across modules. This warrants consideration for a major collection version bump (2.0) rather than a minor increment (1.4.0), to better signal breaking changes or major feature additions to users. While the historical pattern has been 1.x increments, the API moving from v0.x to v1.0 is a natural point for collection versioning to also advance.
Please confirm the versioning strategy: does the maintainers' practice favor semantic versioning tied to the underlying API version, or is there a separate versioning scheme for the collection?
changelogs/changelog.yaml (1)
109-117: LGTM: 1.4.0 release entry is well-structured and accurate.The new release entry correctly documents the upgrade to Flight Control API v1.0, which aligns with the PR objectives. The YAML structure, indentation, and content format are consistent with previous release entries.
tests/unit/plugins/module_utils/fixtures/client.yaml (1)
2-2: LGTM! Organization field correctly positioned at root level.The organization field is now properly placed at the root level, aligning with the configuration schema defined in
config_loader.py(line 51) which expects it as a top-level property. This correction enables the config loader to successfully retrieve the organization value viaconfig_data.get("organization").plugins/doc_fragments/auth.py (1)
33-37: LGTM! Organization parameter documented consistently.The
flightctl_organizationoption follows the established pattern for authentication parameters in this fragment, with environment variable fallback support viaFLIGHTCTL_ORGANIZATION. This will enable organization-scoped requests across all modules that extend this fragment.README.md (2)
59-59: LGTM! Collection version bumped to 1.4.0.The documentation correctly reflects the new collection version 1.4.0, ensuring users can install the version that includes organization support and v1beta1 API updates.
Also applies to: 65-65
88-88: LGTM! API version migration to v1beta1 documented with organization example.The examples have been updated to use
flightctl.io/v1beta1and include a new example demonstrating organization-scoped device creation. This provides clear guidance for users adopting the new API version and organization scoping features.Also applies to: 96-101, 107-109
demo/create.yml (2)
12-12: LGTM! Organization parameter added to credential defaults.The
flightctl_organizationparameter is properly added to the credential defaults withdefault(omit)fallback, making it optional while supporting organization-scoped operations when specified.
46-46: LGTM! API version consistently updated to v1beta1.All resource definitions and module calls have been updated to use
flightctl.io/v1beta1, ensuring consistency across the demo playbook with the API migration.Also applies to: 61-61, 82-82, 102-102, 116-118, 186-186
plugins/module_utils/config_loader.py (5)
27-27: LGTM! Warning callback mechanism properly implemented.The addition of
warn_callbackparameter and the_warnmethod enables the config loader to report warnings to the caller in a flexible way, improving the module's integration with Ansible's warning system.Also applies to: 33-34, 154-156
51-51: LGTM! Organization field added to configuration schema.The
organizationfield is correctly defined as a root-level string property in the configuration schema, enabling organization-scoped requests throughout the collection.
55-57: LGTM! Token configuration options expanded.The schema now supports
access-token,refresh-token, andtoken-to-usefields, providing flexibility in token management and selection strategies.
111-131: LGTM! Token selection logic implements fallback behavior correctly.The token resolution logic properly implements fallback behavior based on the
token-to-useconfiguration:
- When
token-to-useis "refresh", it usesrefresh-tokenif available, falls back toaccess-token, or warns if neither exists.- When
token-to-useis "access", it usesaccess-tokenif available, falls back torefresh-token, or warns if neither exists.- When
token-to-useis not specified, it defaults toaccess-tokenif available.This addresses the concern from the previous review about incomplete fallback logic.
Note: The Ruff static analysis warnings about "hardcoded password" (S105) and
setattrusage (B010) are false positives in this context:
token_to_useis a configuration key name, not a hardcoded password.setattris intentionally used for dynamic attribute assignment based on config file contents.
133-137: LGTM! Organization parsing implemented correctly.The organization value is properly retrieved from the root level of the configuration data and assigned when present, aligning with the schema definition at line 51.
tests/integration/targets/flightctl_certificate_management/tasks/certificate-signing-denial.yml (2)
8-8: LGTM! Organization parameter added to test connection info.The
flightctl_organizationparameter is properly added to the connection info withdefault(omit)fallback, enabling organization-scoped testing while maintaining backward compatibility.
16-16: LGTM! API version updated to v1beta1.The
api_versionhas been correctly updated toflightctl.io/v1beta1, aligning the integration test with the API migration.tests/integration/targets/flightctl_certificate_management/tasks/enrollment-denial.yml (2)
8-8: LGTM! Organization parameter added to test connection info.The
flightctl_organizationparameter is properly added to the connection info withdefault(omit)fallback, enabling organization-scoped testing while maintaining backward compatibility.
16-16: LGTM! API version updated to v1beta1.The
api_versionhas been correctly updated toflightctl.io/v1beta1, aligning the integration test with the API migration.plugins/modules/flightctl_resource.py (1)
27-27: LGTM! Default API version updated to v1beta1.The module's default
api_versionhas been consistently updated fromflightctl.io/v1alpha1toflightctl.io/v1beta1across documentation, examples, and the argument specification. This aligns with the collection-wide API migration to v1beta1.Users should be aware that v1beta1 APIs are in Technical Preview status (as noted in the README), and the default behavior of this module has changed accordingly.
Also applies to: 69-69, 75-75, 130-130
tests/integration/targets/flightctl_resource_info/tasks/devices.yml (2)
8-12: LGTM!The
flightctl_organizationparameter is correctly added to the connection_info anchor withdefault(omit)for optional handling, consistent with other optional parameters likeflightctl_token.
66-76: API version specified only for labeled device tasks.The
api_version: flightctl.io/v1beta1is explicitly set for these device creation tasks but not for other resource tasks (e.g., fleet creation at line 14, device creation at line 29). If intentional (e.g., testing default version fallback), this is fine. Otherwise, consider applying consistently across all resource creation tasks in this test file.plugins/module_utils/api_module.py (1)
428-451: The CSR approval logic correctly uses the valid enum values from the flightctl-python-client library. The updated naming convention (CertificateSigningRequestApproved,CertificateSigningRequestDenied, andConditionStatusTrue) matches the current library definitions. No changes needed..github/workflows/integration-tests.yaml (1)
37-44: Good addition for CI disk space management.The free disk space steps are well-positioned and configured. Using
tool-cache: falsepreserves build tools whilelarge-packages: truefrees significant space. The|| trueon the podman prune ensures graceful failure handling.
2 participants
Summary by CodeRabbit
New Features
Updated API Version
Version Bump
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.