ngine-io / ansible-collection-cloudstack

CloudStack Ansible Collections
https://galaxy.ansible.com/ngine_io/cloudstack
GNU General Public License v3.0
21 stars 28 forks source link

Add VXLAN as isolation method for physical network #73

Closed derJD closed 3 years ago

derJD commented 3 years ago

This small change adds VXLAN as an isolation option.

API documentation states there are only three isolation methods, which might stems from this source here: VLAN, L3 and GRE.

In fact when using the UI there are much more options, which can be seen in this source.

codecov[bot] commented 3 years ago

Codecov Report

Merging #73 (be42582) into master (ce0ea01) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #73   +/-   ##
=======================================
  Coverage   84.36%   84.36%           
=======================================
  Files          53       53           
  Lines        5500     5500           
  Branches     1246     1246           
=======================================
  Hits         4640     4640           
  Misses        429      429           
  Partials      431      431           
Impacted Files Coverage Δ
plugins/modules/cs_physical_network.py 82.55% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ce0ea01...be42582. Read the comment docs.

derJD commented 3 years ago

By adding only VXLAN as method, everything continues to work just fine with this (awesome) collection. It should be okay to add all isolation methods, since the API endpoint has no additional arguments. But this might lead to opening hidden dependencies, like additional unknown steps for cs_zone, cs_vlan_ip_range or cs_traffic_type, when doing that. Oh and I don't know since when they exist, since just started to tinker with cloudstack version 4.15. Tbh. I don't know what half of these isolation methods are doing.

On one hand I have no intentions in testing each and every isolation method if my concerns hold any ground. On the other hand I have no problem in adding all methods and renaming this MR, if you whish.

resmo commented 3 years ago

Okay, so I'll give it more time and merge this in the meanwhile ;) Thanks!