Skip to content

feat(inventory): add polymorphic host classes#87

Draft
Laure-di wants to merge 18 commits intoscaleway:mainfrom
Laure-di:dynamic-inventory
Draft

feat(inventory): add polymorphic host classes#87
Laure-di wants to merge 18 commits intoscaleway:mainfrom
Laure-di:dynamic-inventory

Conversation

@Laure-di
Copy link
Copy Markdown
Contributor

@Laure-di Laure-di commented Sep 23, 2025

How to test:

  1. At the root of the project, run this command:
    ansible-galaxy collection install . --force
  2. create a file inventory.scw.yml
plugin: scaleway.scaleway.scaleway
hostnames:
  - public_ipv4
variables:
  ansible_host: public_ipv4
tags:
  - dedibox #can be change by other tags
  1. In the directory where the file inventory.scw.yml is located:
    ansible-inventory -i inventory.scw.yml --yaml --list

@Laure-di Laure-di requested a review from remyleone as a code owner September 23, 2025 14:35
@Laure-di Laure-di marked this pull request as draft September 23, 2025 14:35
@Laure-di Laure-di self-assigned this Sep 23, 2025
Copy link
Copy Markdown
Contributor

@Gnoale Gnoale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see some abstraction, I think it should be a bit simplified though, you can try to split the main class in smaller unit of work, and you should add a few tests to make sure everything works as expected...

@dataclass
class _ElasticMetalHost(_Host):
def populate_network(self, server: "BaremetalServer", client: "Client") -> None:
for ip in server.ips or []:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

easier to read/understand

if server.ips is None:
  return

for ips in server.ips
...

ips: list[IP] = [
ip
for pn in private_networks_id
for ip in ipam_api.list_i_ps_all(resource_id=pn.id, attached=True)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# If the hostname attribute is a list, create a host for each element
hostnames = hostname_value if isinstance(hostname_value, list) else [hostname_value]

for idx, hostname in enumerate(hostnames):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should probably add a few tests and maybe split the class it seems a bit large no ?

@Mia-Cross Mia-Cross added documentation Improvements or additions to documentation enhancement New feature or request priority:highest Bugs filled by customers, security issues and removed documentation Improvements or additions to documentation enhancement New feature or request priority:highest Bugs filled by customers, security issues labels Oct 6, 2025
@Laure-di Laure-di force-pushed the dynamic-inventory branch 2 times, most recently from 4474481 to 696f708 Compare October 7, 2025 17:33
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.

3 participants