NO-ISSUE: Update ansible-collection to v1.6#58
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughUpgrades the Ansible collection from v1.5.0 to v1.6.0 to support Flight Control API v1.2. The Changesv1.6.0 Release: API v1.2 Client Upgrade with URL Normalization and Org_id Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (8 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/module_utils/api_module.py (1)
125-141: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueURL normalization logic assumes clean base URL structure.
The logic strips trailing slashes and conditionally appends
/api/v1to construct the normalized host. For typical use cases (base URLs likehttps://example.com:8080/), this works correctly and handles the test fixtures as designed.However, the check
if not host_url.endswith('/api/v1')does not validate URL structure. If a user provides a URL with/api/v1embedded in a path component (e.g.,https://example.com/api/v1/extra/), the logic would incorrectly append another/api/v1, resulting in a malformed URL.Consider adding a defensive check to validate that the final URL has the expected structure, or document the assumption that base URLs should not contain
/api/v1in intermediate path segments.💡 Optional defensive enhancement
host_url = self.url.geturl().rstrip('/') if not host_url.endswith('/api/v1'): host_url = f"{host_url}/api/v1" # Optional: validate that the normalized URL has the expected structure # (prevents accidental double-pathing if input already contains /api/v1) if host_url.count('/api/v1') != 1: raise FlightctlException(f"Invalid host URL structure: {self.url.geturl()}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/module_utils/api_module.py` around lines 125 - 141, The URL normalization logic that constructs host_url does not validate that the final URL has the expected structure, which could result in malformed URLs with duplicate `/api/v1` segments if the input URL already contains this path component. After the host_url is normalized (where the rstrip and conditional append of `/api/v1` occur), add a validation check to ensure that `/api/v1` appears exactly once in the final URL. If the count is not equal to 1, raise a FlightctlException with a descriptive error message that includes the original URL from self.url.geturl(). This defensive check should validate the normalized host_url before it is used to create the Configuration objects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changelogs/changelog.yaml`:
- Around line 138-145: Create the missing changelog fragment file referenced in
the 1.6.0 release entry. The changelog entry references the fragment file
`flightctl_1.6.0_update.yml` which must be created in the fragments directory.
Structure this fragment file using the appropriate YAML format with the
`major_changes` category to match the changelog entry for version 1.6.0,
ensuring it contains the content describing the support for Flight Control API
v1.2 to satisfy the `antsibull-changelog release` command requirements.
In `@requirements.txt`:
- Line 5: The flightctl dependency in requirements.txt is referencing a
non-existent git tag (pc_1.2) from a personal fork, which will cause build
failures. Replace the entire git-based dependency specification on the flightctl
line with the official published PyPI package by changing it to use
flightctl-client==1.1.0 or a higher version if available on PyPI. This ensures
supply chain integrity and uses the officially maintained package rather than a
personal repository with non-existent tags.
---
Outside diff comments:
In `@plugins/module_utils/api_module.py`:
- Around line 125-141: The URL normalization logic that constructs host_url does
not validate that the final URL has the expected structure, which could result
in malformed URLs with duplicate `/api/v1` segments if the input URL already
contains this path component. After the host_url is normalized (where the rstrip
and conditional append of `/api/v1` occur), add a validation check to ensure
that `/api/v1` appears exactly once in the final URL. If the count is not equal
to 1, raise a FlightctlException with a descriptive error message that includes
the original URL from self.url.geturl(). This defensive check should validate
the normalized host_url before it is used to create the Configuration objects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b45e4ae4-3201-4a0c-9b1e-c753c49d0065
📒 Files selected for processing (8)
CHANGELOG.rstREADME.mdchangelogs/changelog.yamlgalaxy.ymlplugins/module_utils/api_module.pyrequirements.txttests/integration/requirements.txttests/unit/requirements.txt
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
changelogs/changelog.yaml (1)
138-145:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFragment file missing—release workflow will fail. [DUPLICATE]
This is a repeat of the critical issue already flagged in the previous review: the changelog entry references fragment
flightctl_1.6.0_update.yml, which does not exist inchangelogs/fragments/. Theantsibull-changelog releasecommand requires all referenced fragments to exist before release can proceed.Create the fragment file
changelogs/fragments/flightctl_1.6.0_update.ymlin YAML format with themajor_changescategory to match this release entry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@changelogs/changelog.yaml` around lines 138 - 145, The changelog.yaml file references a fragment file flightctl_1.6.0_update.yml that does not exist in the changelogs/fragments/ directory, which will cause the antsibull-changelog release command to fail. Create the missing fragment file flightctl_1.6.0_update.yml in the changelogs/fragments/ directory as a YAML file with a major_changes category entry that describes the Flight Control API version 1.2 update to match the content referenced in the changelog entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/inventory/flightctl.py`:
- Line 433: The wrapper function param_serialize_with_org_id is missing explicit
type annotations for its parameters and return type, which violates the codebase
typing guidelines and creates inconsistency with the enclosing function that is
fully typed. Add type hints to the function signature for *args and **kwargs
parameters, and specify the return type annotation to match the typing pattern
used throughout the rest of the codebase and maintain consistency with the
parent function's type safety.
---
Duplicate comments:
In `@changelogs/changelog.yaml`:
- Around line 138-145: The changelog.yaml file references a fragment file
flightctl_1.6.0_update.yml that does not exist in the changelogs/fragments/
directory, which will cause the antsibull-changelog release command to fail.
Create the missing fragment file flightctl_1.6.0_update.yml in the
changelogs/fragments/ directory as a YAML file with a major_changes category
entry that describes the Flight Control API version 1.2 update to match the
content referenced in the changelog entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b4f2245e-0690-4296-89a8-702f7b707a51
📒 Files selected for processing (9)
CHANGELOG.rstREADME.mdchangelogs/changelog.yamlgalaxy.ymlplugins/inventory/flightctl.pyplugins/module_utils/api_module.pyrequirements.txttests/integration/requirements.txttests/unit/requirements.txt
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/unit/requirements.txt (1)
5-5:⚠️ Potential issue | 🔴 CriticalSame critical supply chain issue as requirements.txt.
This file contains the same problematic
flightctl-clientdependency referencing the non-existent git tagpc_1.2. Unit tests cannot proceed until the main requirements.txt is corrected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/requirements.txt` at line 5, The flightctl-client dependency in tests/unit/requirements.txt references a non-existent git tag `pc_1.2` which prevents dependency resolution. Update the git reference in the dependency specification to use a valid and existing git tag, branch name, or commit SHA from the flightctl-python-client repository. Verify the corrected reference resolves properly before committing the change.tests/integration/requirements.txt (1)
3-3:⚠️ Potential issue | 🔴 CriticalSame critical supply chain issue as requirements.txt.
This file contains the same problematic
flightctl-clientdependency referencing the non-existent git tagpc_1.2. Integration tests cannot proceed until the main requirements.txt is corrected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/requirements.txt` at line 3, The flightctl-client dependency in the integration test requirements file references a non-existent git tag pc_1.2, preventing integration tests from running. Update the git reference in the flightctl-client @ git+https://github.com/SiddarthR56/flightctl-python-client.git@pc_1.2 dependency to point to a valid and existing git tag or branch instead of the non-existent pc_1.2 tag. Ensure the tag or branch you reference actually exists in the repository.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/integration-tests.yaml:
- Line 14: The FLIGHTCTL_REF environment variable update to v1.2.0 in the
integration-tests.yaml workflow is blocked by a dependency issue. Before merging
this workflow change, identify and fix the flightctl-client dependency
declaration in requirements.txt that is currently failing during CI dependency
installation. Resolve the flightctl-client issue first so that when the workflow
runs, the Python client dependency can be successfully installed alongside the
updated FLIGHTCTL_REF version.
In `@requirements.txt`:
- Line 5: The flightctl-client dependency in requirements.txt is specified as a
git URL pointing to a non-existent tag (pc_1.2) in a personal fork, which breaks
reproducible builds and supply chain integrity. Replace the entire line with the
official PyPI package pinned to a released version: flightctl-client==1.1.0 (the
latest stable version available on PyPI). This ensures deterministic builds and
uses the officially maintained package source.
---
Duplicate comments:
In `@tests/integration/requirements.txt`:
- Line 3: The flightctl-client dependency in the integration test requirements
file references a non-existent git tag pc_1.2, preventing integration tests from
running. Update the git reference in the flightctl-client @
git+https://github.com/SiddarthR56/flightctl-python-client.git@pc_1.2 dependency
to point to a valid and existing git tag or branch instead of the non-existent
pc_1.2 tag. Ensure the tag or branch you reference actually exists in the
repository.
In `@tests/unit/requirements.txt`:
- Line 5: The flightctl-client dependency in tests/unit/requirements.txt
references a non-existent git tag `pc_1.2` which prevents dependency resolution.
Update the git reference in the dependency specification to use a valid and
existing git tag, branch name, or commit SHA from the flightctl-python-client
repository. Verify the corrected reference resolves properly before committing
the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 83c8b23c-7838-4ffa-a1be-f1d3a56b51f2
📒 Files selected for processing (4)
.github/workflows/integration-tests.yamlrequirements.txttests/integration/requirements.txttests/unit/requirements.txt
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/integration-tests.yaml:
- Around line 49-50: Replace the container image reference
`quay.io/flightctl/e2eregistry:2` with a pinned full SHA256 digest format
(`quay.io/flightctl/e2eregistry@sha256:...`) to ensure reproducibility and
prevent upstream retags from silently changing CI behavior. Apply this same
change in both the integration-tests job (around line 50 in the "Start e2e
registry" step) and the test-integration-downstream job (around line 104) to
maintain consistency across both jobs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 691ef336-c74a-482c-8d27-fc5e166b27e4
📒 Files selected for processing (12)
.github/workflows/integration-tests.yamlCHANGELOG.rstREADME.mdchangelogs/changelog.yamlgalaxy.ymlplugins/inventory/flightctl.pyplugins/module_utils/api_module.pyrequirements.txttests/integration/requirements.txttests/integration/targets/flightctl_image_builder/tasks/imagebuild-lifecycle.ymltests/integration/targets/flightctl_image_builder/tasks/imageexport-lifecycle.ymltests/unit/requirements.txt
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/integration-tests.yaml (1)
50-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin
e2e-registryimage to an immutable digest (Major).Using
quay.io/flightctl/e2eregistry:2on Line 50 and Line 104 allows upstream retags, so CI can run different code over time (non-reproducible + supply-chain drift risk).Suggested fix
- run: docker run -d --name e2e-registry --network kind -p 5000:5000 quay.io/flightctl/e2eregistry:2 + run: docker run -d --name e2e-registry --network kind -p 5000:5000 quay.io/flightctl/e2eregistry@sha256:<pinned_digest>Also applies to: 104-104
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/integration-tests.yaml at line 50, The docker image reference quay.io/flightctl/e2eregistry:2 uses a mutable tag which allows upstream retags, creating non-reproducible builds and supply chain risks. Replace the image tag :2 with an immutable SHA256 digest on both the docker run command at line 50 and the other occurrence at line 104. Instead of quay.io/flightctl/e2eregistry:2, use the full image reference with the SHA256 digest (format: quay.io/flightctl/e2eregistry@sha256:...) to ensure the exact same image is always pulled in CI runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.github/workflows/integration-tests.yaml:
- Line 50: The docker image reference quay.io/flightctl/e2eregistry:2 uses a
mutable tag which allows upstream retags, creating non-reproducible builds and
supply chain risks. Replace the image tag :2 with an immutable SHA256 digest on
both the docker run command at line 50 and the other occurrence at line 104.
Instead of quay.io/flightctl/e2eregistry:2, use the full image reference with
the SHA256 digest (format: quay.io/flightctl/e2eregistry@sha256:...) to ensure
the exact same image is always pulled in CI runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: d52086ba-72c2-4b49-a6ca-ac84c852a226
📒 Files selected for processing (12)
.github/workflows/integration-tests.yamlCHANGELOG.rstREADME.mdchangelogs/changelog.yamlgalaxy.ymlplugins/inventory/flightctl.pyplugins/module_utils/api_module.pyrequirements.txttests/integration/requirements.txttests/integration/targets/flightctl_image_builder/tasks/imagebuild-lifecycle.ymltests/integration/targets/flightctl_image_builder/tasks/imageexport-lifecycle.ymltests/unit/requirements.txt
| jsonschema | ||
| websockets>=15.0.1 | ||
| flightctl-client==1.1.0 | ||
| flightctl-client==1.2.1 |
There was a problem hiding this comment.
we are at 1.2.0 version AFAIK
There was a problem hiding this comment.
We are effectively at 1.2.0. There was an issue with the Python package upload, and after deleting it I couldn't re-upload the same version because PyPI doesn't allow reusing a version number. As a result, I had to publish it as 1.2.1, but there are no functional changes from 1.2.0—the version bump was only to work around the PyPI restriction.
|
@SiddarthR56 I noticed certificate tests failed Failed to sign certificate: error signing certificate - FlightCtl v1.2.x introduced validation: For CertificateSigningRequest (CSR), the Common Name (CN) in the certificate request MUST match metadata.name. Current test has: metadata.name: ansible-integration-test-approval-csr full test output https://privatebin.corp.redhat.com/?f52208450a0a5eed#EirSSqwN1RH5pw9wjJDPtk7wypAgyadRVbaodpNFR2am |
|
@SiddarthR56 I noticed imagebuild tests failed as well ... |
Assisted-by: Cursor/Claude
3 participants
Summary
This PR releases collection version 1.6.0 and updates the underlying Flight Control client dependency to support Flight Control API v1.2. It improves base URL handling by normalizing hosts to consistently use
/api/v1, enhances request behavior by optionally scoping requests to an organization via anorg_idquery parameter (inventory plugin), updates CI to pull a newer Flight Control e2e reference and run ane2e-registrylocally, and adjusts integration tests to use that registry over HTTP (removing prior TLS-bypass behavior).Areas Affected
Collection metadata / release docs
galaxy.yml:version: 1.5.0 → 1.6.0CHANGELOG.rst: added v1.6.0 release entry (“support Flight Control API v1.2” and a Major Changes bullet)changelogs/changelog.yaml: addedreleases.1.6.0withrelease_summary, reference toflightctl_1.6.0_update.yml, andrelease_date: '2026-06-22'README.md: pinned examples toflightctl.core:1.6.0(from1.5.0)Shared utilities (module client setup)
plugins/module_utils/api_module.pyhost_urlby trimming trailing/and ensuring it ends with/api/v1host_urlwhen constructingConfiguration(...)andV1Alpha1Configuration(...)Inventory plugin behavior
plugins/inventory/flightctl.pyhostto end with/api/v1config.organizationis set, scopes outgoing requests by appendingorg_id=<organization>(only when anorg_idquery param is not already present)_set_org_id_query_param(client, organization)Dependencies / test environment requirements
requirements.txt:flightctl-client 1.1.0 → 1.2.1tests/unit/requirements.txt:flightctl-client 1.1.0 → 1.2.1tests/integration/requirements.txt:flightctl-client 1.1.0 → 1.2.1CI configuration
.github/workflows/integration-tests.yamlenv.FLIGHTCTL_REFtov1.2.0(fromv1.1.1)make deploy-e2e-extraswith starting ane2e-registrycontainer:docker run -d --name e2e-registry --network kind -p 5000:5000 quay.io/flightctl/e2eregistry:2Integration tests
tests/integration/targets/flightctl_image_builder/tasks/imagebuild-lifecycle.ymle2e-registry:5000withscheme: httpskipServerVerification: truetests/integration/targets/flightctl_image_builder/tasks/imageexport-lifecycle.ymle2e-registry:5000withscheme: httpskipServerVerification: trueModule API surface / backward-compatibility implications
/or without/api/v1(now normalized more consistently).organizationis configured, requests will includeorg_id, potentially changing server-side results.