Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add metal_port module #205

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

feat: add metal_port module #205

wants to merge 15 commits into from

Conversation

ctreatma
Copy link
Contributor

@ctreatma ctreatma commented Aug 29, 2024

Closes #61.

This introduces a metal_port module so that Ansible collection users can modify the network port configuration of their Metal devices. The metal_port Ansible module is based heavily on the metal_port Terraform resource.

Device ports cannot be created or deleted, and updating a device port involves conditionally making multiple different API requests, so the metal_port module does not fit the assumptions made by the metal utility package of this collection. In light of that, the metal_port module has been written in the style of the amazon.aws collection. In that style, each Ansible module uses an API client directly rather than offloading to a shared package that encapsulates all API requests.

if len(vlan_assignments) > 0:
port = _create_and_wait_for_batch(module, port, vlan_assignments, 1800)

# 8: update native VLAN ID
Copy link
Member

@displague displague Sep 13, 2024

Choose a reason for hiding this comment

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

Any reason native is not assigned/unassigned/reassigned in batch via the native flag?

https://deploy.equinix.com/developers/api/metal/#tag/Ports/operation/createPortVlanAssignmentBatch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handling native in the batch increases the complexity of the code to construct the batch, with minimal benefit (one less API request).

@ocobles
Copy link

ocobles commented Sep 16, 2024

maybe it would be nice to translate the examples that are defined in https://registry.terraform.io/providers/equinix/equinix/latest/docs/guides/network_types#metal-port to add them in these ansible docs since it is not very obvious the different combinations needed to achieve each mode

plugins/modules/metal_port.py Outdated Show resolved Hide resolved


specdoc_examples = [
'''
Copy link
Member

Choose a reason for hiding this comment

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

Is this formatting what black or other linters would product? (with ''' not in the same column)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how the module template is formatted (and as a result, every other module in this collection). We may want to change this and (add a linter and/or formatter) but that's a bigger scope than just one module.

plugins/modules/metal_port.py Outdated Show resolved Hide resolved
- [Return Values](#return-values)

## Examples

Copy link
Member

@displague displague Sep 16, 2024

Choose a reason for hiding this comment

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

I agree with what @ocobles pointed out, that we could benefit from the test cases as examples with some contextual text, similar to https://registry.terraform.io/providers/equinix/equinix/latest/docs/guides/network_types#metal-port

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The example string specified in the code is dumped into a fenced code block, so we don't have a lot of flexibility on how that's rendered at the moment. I added more example tasks with descriptive names to the example string. We could add more text in YAML comments inside the example block, but IMO that's not a great experience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have one richer Markdown example for the collection, although I don't see anything that links to or references it: https://github.com/equinix/ansible-collection-equinix/tree/main/examples/device_assign_ip

Something like that would be good for metal_port, but I'd prefer to add that in a separate PR so we can take some time to think about how and where to reference it.

@displague
Copy link
Member

So many tests! 🚀

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.

metal_port
4 participants