-
Notifications
You must be signed in to change notification settings - Fork 1
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: metal virtual circuit #141
feat: metal virtual circuit #141
Conversation
@antoninrykalsky Thanks for the PR. We will wait for an Interconnection to test this properly. |
Heads up that there is an upcoming requirement for VLAN in the requests: https://feedback.equinixmetal.com/changelog/virtual-circuits-will-require-vlans-on-feb-29-2024 Since this PR allows for |
The e2e tests would be possible, reusing the same dedicated connection used by Terraform e2e testing. We are a little worried about racing between the two tests. This may not be problematic if Terraform created VCs on the dedicated connection have no impact on Ansible created VCs (resources are swept accordingly by name prefixes, and VC quantity is not impacted by parallel test runners). We can probably run with this and approach and keep an eye out for test failures as described. @ctreatma suggested that we could make this test conditional based on a dedicated connection environment variable being defined, Alternatively, dedicated connection mocks would be helpful here because dedicated connections have harder setup requirements and we don't want to accidentally delete them or misconfigure them. |
I'd like to be able to include this in the v0.4.0 release in the next few weeks. |
|
It should be possible to test reading in the virtual circuit returned by a shared interconnection. |
('metal_virtual_circuit', action.CREATE): spec_types.Specs( | ||
equinix_metal.InterconnectionsApi(mpc).create_interconnection_port_virtual_circuit, | ||
{'connection_id': 'organization_id', 'port_id': 'port_id'}, | ||
equinix_metal.VirtualCircuitCreateInput, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unfamiliar with the patterns in this project.
Is L279 correct?
Is L280 expected to work with the single CreateInput type or perhaps does this resource need to be split up like metal_connection_project_*
to accommodate the multiple input types: VlanVirtualCircuitCreateInput
,VrfVirtualCircuitCreateInput
?
@displague Can you please check that ANSIBLE_ACC_METAL_DEDICATED_CONNECTION_ID was indeed set? Is it in the same "namespace" as the access token? We need to read it as envvar and it seems empty. |
This PR adds secret ANSIBLE_ACC_METAL_DEDICATED_CONNECTION_ID to integration test pr as an env var so that we can use it in #141
|
Needs a rebase @antoninrykalsky |
d8f3568
to
6976b11
Compare
5d1423c
to
d1e94b5
Compare
d1e94b5
to
b25d73e
Compare
@antoninrykalsky the metal-python SDK was recently upgraded and this will need a rebase to pull in those changes. The upgraded SDK is able to differentiate between VLAN and VRF virtual circuits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@antoninrykalsky lgtm but needs a rebase.
d36020b
to
be348c7
Compare
This PR is included in version 0.6.0 🎉 |
Fixes #70
Fixes #33