-
Notifications
You must be signed in to change notification settings - Fork 458
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
add support for setting SRVs for network #460
Conversation
Thx look good so far, i would need to have look more later. (Looked at it with phone). I think you missed the dpc part here https://github.com/dmacvicar/terraform-provider-libvirt/blob/master/website/docs/r/network.markdown |
@MalloZup updated to add docs in |
Globally looks good, thx!! I might have look on the comments on the vars , once i have time. E.g if there is nonot solution for that we might create issue here ans upstream |
}, | ||
"proto": { | ||
Type: schema.TypeString, | ||
// This should be required, but Terraform does validation too early |
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.
Referring to this
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.
This is the same issue we faced when adding dns.hosts
in #382
similar comment on https://github.com/dmacvicar/terraform-provider-libvirt/pull/460/files/de77bbb1d412cac90118176391ba9d349d1a38cc#diff-042e2374df248e4277a3506b065eb28aR174
i'm not 100% on terraform but, it looks like terraform complains in plan phase when the list from dns srv template has not been computed.
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.
Yop, i think we might have an issue just for remember it and fix it later. I will havw a look once i have terminal :)
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.
Yop, i think we might have an issue just for remember it and fix it later. I will havw a look once i have terminal :)
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.
👍 Thanks!
Type: schema.TypeString, | ||
Required: true, | ||
}, | ||
"proto": { |
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.
Why did you used proto when the libvirt equivalent is protocol?
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.
Oops i mxed up names from DNS SRV RFC 2782 and NetworkDNSSRV
fixed to match all names to NetworkDNSSRV
Type: schema.TypeMap, | ||
Computed: true, | ||
}, | ||
}, |
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.
Is there a reason some of the attributes present in NetworkDNSSRV are left out?
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.
Added the one missing field priority
from NetworkDNSSRV
This commit adds a new template for generating dns srv records similar to dns hosts and then adds `srvs` key to `dns` section for libvirt_network. ```hcl a libvirt network DNS SRV template datasource Datasource example: data "libvirt_network_dns_srv_template" "etcd_cluster" { count = "${var.etcd_count}" service = "etcd-server" protocol = "tcp" domain = "${discovery_domain}" target = "${var.cluster_name}-etcd-${count.index}.${discovery_domain}" } resource "libvirt_network" "k8snet" { ... dns = [{ srvs = [ "${flatten(data.libvirt_network_dns_srv_template.etcd_cluster.*.rendered)}" ] }] ... } ```
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.
looks good to me. thx!
@dmacvicar it would be awesome if you can take a second pass :). |
dmacvicar/terraform-provider-libvirt#460 is pending approval, so in the mean time changing the docs to point to forked repo for getting DNS SRV changes.
@abhinavdahiya thx ! |
This reverts commit 0443add. dmacvicar/terraform-provider-libvirt#460 was merged people still need to update their local builds..
I am getting this on the acceptance tests:
Which is clearly missing quotes on: func TestAccLibvirtNetworkDataSource_DNSSRVTemplate(t *testing.T) {
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Steps: []resource.TestStep{
{
Config: `data "libvirt_network_dns_srv_template" "etcd_cluster" {
count = 2
service = etcd-server-ssl
protocol = tcp
target = "my-etcd-${count.index}.tt.testing"
}`,
Check: resource.ComposeTestCheckFunc( I will push a fix, as it is blocking me from developing another branch. We need urgently to setup acceptance tests in CI somehow. |
@dmacvicar yop i agree with that. at least it can be also annoying if we need to run for each PR from contributors the testacc |
Fix broken acceptance tests from #460
This commit adds a new template for generating dns srv records similar to dns hosts
and then adds
srvs
key todns
section for libvirt_network.