Skip to content

Conversation

@dguido
Copy link
Member

@dguido dguido commented Nov 28, 2025

Summary

  • Replace 3 custom modules with official Ansible collection equivalents
  • Remove 519 lines of custom code
  • Add linode.cloud and community.digitalocean collections

Changes

Custom Module Replacement Lines Removed
linode_v4.py linode.cloud.instance 131
linode_stackscript_v4.py linode.cloud.stackscript 102
digital_ocean_floating_ip.py community.digitalocean.digital_ocean_floating_ip 278

Files Modified

  • requirements.yml - Added linode.cloud>=0.41.0 and community.digitalocean>=1.26.0
  • roles/cloud-linode/tasks/main.yml - Updated to FQCN, renamed access_tokenapi_token
  • roles/cloud-digitalocean/tasks/main.yml - Updated to FQCN
  • .ansible-lint - Removed 3 entries from mock_modules

Modules Kept (see #14902 comment for details)

  • x25519_pubkey.py - No collection equivalent (crypto-specific)
  • scaleway_compute.py - Deferred (complex state machine, recently fixed)
  • lightsail_region_facts.py - No collection equivalent exists
  • gcp_compute_location_info.py - No equivalent; google.cloud being deprecated

Test plan

  • ansible-galaxy collection install -r requirements.yml --force
  • ansible-lint . - Passed
  • ansible-playbook main.yml --syntax-check - Passed
  • ansible-playbook users.yml --syntax-check - Passed
  • pytest tests/unit/ -q - 91 passed
  • Integration test with Linode account (manual)
  • Integration test with DigitalOcean account (manual)

Closes #14902

🤖 Generated with Claude Code

Replace custom modules with official Ansible collection equivalents:
- linode_v4 → linode.cloud.instance
- linode_stackscript_v4 → linode.cloud.stackscript
- digital_ocean_floating_ip → community.digitalocean.digital_ocean_floating_ip

Changes:
- Add linode.cloud and community.digitalocean to requirements.yml
- Update roles/cloud-linode/tasks/main.yml (FQCN + api_token param)
- Update roles/cloud-digitalocean/tasks/main.yml (FQCN)
- Remove 3 entries from .ansible-lint mock_modules
- Delete 3 custom modules (519 lines removed)

Modules kept (no collection equivalent or deferred):
- x25519_pubkey.py - crypto-specific, no equivalent
- scaleway_compute.py - complex state machine, deferred
- lightsail_region_facts.py - no collection equivalent
- gcp_compute_location_info.py - no equivalent, google.cloud deprecated

Closes #14902

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@dguido dguido requested a review from jackivanov as a code owner November 28, 2025 22:24
@claude
Copy link
Contributor

claude bot commented Nov 28, 2025

Code Review

Summary

This PR successfully migrates 3 custom Ansible modules to official collection equivalents, removing 519 lines of custom code. The changes are well-structured and follow the project's conventions.

✅ Strengths

  1. Excellent code reduction: Removing 519 lines of custom module code reduces maintenance burden and attack surface
  2. Proper FQCN usage: All modules use fully-qualified collection names (FQCN)
  3. Correct .ansible-lint updates: Properly removed the 3 modules from mock_modules list
  4. Clear documentation: PR description includes comprehensive testing checklist and rationale for modules kept
  5. Parameter migration: Correctly updated access_tokenapi_token for Linode modules

🔍 Observations

Version Constraints

The requirements.yml uses different version constraint strategies:

# Exact versions (existing pattern)
- name: ansible.posix
  version: "==2.1.0"

# Minimum versions (new additions)
- name: linode.cloud
  version: ">=0.41.0"
- name: community.digitalocean
  version: ">=1.26.0"

Question: Is there a reason for using >= instead of == for the new collections? The project philosophy emphasizes "Stability over features" and existing collections use exact versions. Consider:

  • Using exact versions for reproducible builds
  • Adding these to uv.lock or similar lockfile mechanism

Parameter Compatibility

The parameter rename access_tokenapi_token for Linode modules is applied consistently:

  • roles/cloud-linode/tasks/main.yml:14 - stackscript module
  • roles/cloud-linode/tasks/main.yml:41 - instance module

Verification needed: Confirm that linode.cloud>=0.41.0 uses api_token (not access_token) to avoid runtime failures.

DigitalOcean Module Compatibility

The community.digitalocean.digital_ocean_floating_ip module should be API-compatible with the custom version, but the PR would benefit from verification that:

  1. The oauth_token parameter name is still correct (not changed to api_token)
  2. The return data structure matches (especially data.floating_ip.ip path used in line 43)

🧪 Testing Recommendations

Since integration tests require cloud accounts, consider:

  1. Dry-run validation: Can you run with --check mode against a test inventory?
  2. Module documentation: Verify parameter names with:
    ansible-doc linode.cloud.stackscript
    ansible-doc linode.cloud.instance
    ansible-doc community.digitalocean.digital_ocean_floating_ip

📋 Checklist Verification

According to CLAUDE.md, before merging:

  • All linters pass (claimed in PR description)
  • Syntax checks pass (claimed in PR description)
  • Unit tests pass (91 passed - ✓)
  • Integration tests (manual - pending)

🎯 Recommendation

Approve with suggestions: The code changes are clean and well-executed. Before merging:

  1. Consider standardizing version constraints to exact versions for stability
  2. Verify parameter names with ansible-doc or collection documentation
  3. Test on at least one cloud provider to confirm runtime compatibility

The structural changes are sound and align with the project's goal of reducing custom code. The risk is low since the changes are isolated to two cloud provider roles.


Overall Assessment: This is a high-quality refactoring that meaningfully improves the codebase. The careful documentation and systematic approach demonstrate good engineering practice. 🚀

@dguido
Copy link
Member Author

dguido commented Nov 28, 2025

I ran a deploy and it worked on DO

@dguido dguido merged commit 42aa65d into master Nov 28, 2025
27 checks passed
@dguido dguido deleted the migrate-to-ansible-collections branch November 28, 2025 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deprecate custom library modules in favor of Ansible collections

2 participants