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.

Sign up for GitHub

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

Draft: Add tests for importing templates #357

Closed
wants to merge 13 commits into from
Closed

Draft: Add tests for importing templates #357

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
97b16d2
Add tests for importing templates
Oct 10, 2022
474f3c3
Remove hardcoded fields, review
Oct 15, 2022
a2bcb88
Merge branch 'OpenNebula:master' into master
thijsvandergugten Dec 8, 2022
149703c
Update template.html.markdown
lzadjsf Dec 9, 2022
039784a
B #382: fix datastore system type
treywelsh Dec 16, 2022
5de5119
B #385: fix host types case
treywelsh Dec 16, 2022
e0d077e
Merge from upstream
Jan 12, 2023
29981c6
B #312: less null values
treywelsh Dec 14, 2022
796e718
B #369: fill template_tags with empty value
treywelsh Dec 14, 2022
b1cf5b7
fix doc
treywelsh Jan 10, 2023
d229fa4
B #389: manage cluster membership from cluster resource
treywelsh Jan 11, 2023
8c1518b
Merge branch 'OpenNebula:master' into master
thijsvandergugten Jan 12, 2023
c8b1100
Merge branch 'OpenNebula:master' into master
thijsvandergugten Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions opennebula/resource_opennebula_template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,9 +424,49 @@ func resourceOpennebulaTemplateReadCustom(ctx context.Context, d *schema.Resourc
d.Set("gid", tpl.GID)
d.Set("uname", tpl.UName)
d.Set("gname", tpl.GName)
d.Set("group", tpl.GName)
d.Set("reg_time", tpl.RegTime)
d.Set("description", "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised that you set an empty value, you could retrieve the description from the template via

desc, err := tpl.Get(vmk.Description)
...

Copy link
Author

Choose a reason for hiding this comment

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

The fields description and sched_requirements were difficult to handle: if this field is removed from the Terraform-resource-configuration, it does not get removed from Terraform-state (it has an empty value, which gives problems in the unit-tests). I will write a new commit with an update of the method flattenVMUserTemplate: in this method, the description-field is already taken care of

d.Set("sched_requirements", "")
d.Set("permissions", permissionsUnixString(*tpl.Permissions))

getTags := make(map[string]interface{}, 0)
for k, v := range pairsToMap(tpl.Template.Template) {
if k != "MEMORY" && k != "CPU" && k != "VCPU" && k != "VROUTER" && k != "DESCRIPTION" && k != "SCHED_REQUIREMENTS" {
getTags[strings.ToLower(k)] = v
}
}
d.Set("tags", getTags)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we don't need this, tags are already read in the flattenVMUserTemplate method called later in the code.
There is some parts of the code that are in methods due to the fact that are shared between resources.
The template resource share some code with the resources: VM, virtual router instance, and virtual router instance template

Copy link
Author

Choose a reason for hiding this comment

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

In the method flattenVMUserTemplate there was a line if tagsInterface, ok := d.GetOk("tags"); ok {. Because of this, only tags which already were present in the state will be processed. I will add a commit where the method flattenVMUserTemplate is updated: it will have a loop over all elements in the template, instead of over all elements in the state.


for _, value := range tpl.Template.Elements {
if pair, ok := value.(*dyn.Pair); ok {
switch pair.XMLName.Local {
case "DESCRIPTION", "SCHED_REQUIREMENTS":
d.Set(strings.ToLower(pair.XMLName.Local), pair.Value)
}
}
if vector, ok := value.(*dyn.Vector); ok {
getOS := make(map[string]interface{}, 0)
switch vector.XMLName.Local {
case "OS", "FEATURES", "GRAPHICS", "CPU_MODEL":
for _, pair := range vector.Pairs {
getOS[strings.ToLower(pair.XMLName.Local)] = pair.Value
}
d.Set(strings.Replace(strings.ToLower(vector.XMLName.Local), "_", "", -1), append(make([]interface{}, 0), getOS))
Copy link
Collaborator

@treywelsh treywelsh Oct 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Please don't call to much methods in the same line this make you code more difficult to read, it's not a problem to add more code lines to make the code more readable/maintainable.

However you get how to read TypeSet elements OS, FEATURES etc. that's a good point

Copy link
Author

Choose a reason for hiding this comment

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

This long line has been separated in multiple lines, and an explanation has been added.

case "CONTEXT":
for _, pair := range vector.Pairs {
switch pair.XMLName.Local {
case "SET_HOSTNAME", "NETWORK":
getOS[pair.XMLName.Local] = pair.Value
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reuse getOS for the context make this code part less understandable.
You could just rename getOS for something more generic like getVector
Or you could instead define a new map in each case of the switch.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, now the name getVector is used.

default:
getOS[strings.ToLower(pair.XMLName.Local)] = pair.Value
}
}
d.Set(strings.ToLower(vector.XMLName.Local), getOS)
}
}
}

err = flattenTemplateDisks(d, &tpl.Template)
if err != nil {
diags = append(diags, diag.Diagnostic{
Expand Down
48 changes: 44 additions & 4 deletions opennebula/resource_opennebula_template_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,18 @@ func TestAccTemplate(t *testing.T) {
resource.TestCheckResourceAttr("opennebula_template.template", "permissions", "660"),
resource.TestCheckResourceAttr("opennebula_template.template", "group", "oneadmin"),
resource.TestCheckResourceAttr("opennebula_template.template", "cpu", "0.5"),
resource.TestCheckResourceAttr("opennebula_template.template", "vcpu", "1"),
resource.TestCheckResourceAttr("opennebula_template.template", "memory", "512"),
resource.TestCheckResourceAttr("opennebula_template.template", "graphics.#", "1"),
resource.TestCheckResourceAttr("opennebula_template.template", "graphics.0.keymap", "en-us"),
resource.TestCheckResourceAttr("opennebula_template.template", "graphics.0.listen", "0.0.0.0"),
resource.TestCheckResourceAttr("opennebula_template.template", "graphics.0.type", "VNC"),
resource.TestCheckResourceAttr("opennebula_template.template", "os.#", "1"),
resource.TestCheckResourceAttr("opennebula_template.template", "os.0.arch", "x86_64"),
resource.TestCheckResourceAttr("opennebula_template.template", "os.0.boot", ""),
resource.TestCheckResourceAttr("opennebula_template.template", "context.%", "2"),
resource.TestCheckResourceAttr("opennebula_template.template", "context.dns_hostname", "yes"),
resource.TestCheckResourceAttr("opennebula_template.template", "context.NETWORK", "YES"),
resource.TestCheckResourceAttr("opennebula_template.template", "tags.%", "2"),
resource.TestCheckResourceAttr("opennebula_template.template", "tags.env", "prod"),
resource.TestCheckResourceAttr("opennebula_template.template", "tags.customer", "test"),
Expand All @@ -55,20 +60,30 @@ func TestAccTemplate(t *testing.T) {
}),
),
},
{
ResourceName: "opennebula_template.template",
ImportState: true,
ImportStateVerify: true,
},
{
Config: testAccTemplateCPUModel,
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("opennebula_template.template", "name", "terra-tpl-cpumodel"),
resource.TestCheckResourceAttr("opennebula_template.template", "permissions", "660"),
resource.TestCheckResourceAttr("opennebula_template.template", "group", "oneadmin"),
resource.TestCheckResourceAttr("opennebula_template.template", "cpu", "0.5"),
resource.TestCheckResourceAttr("opennebula_template.template", "vcpu", "1"),
resource.TestCheckResourceAttr("opennebula_template.template", "memory", "512"),
resource.TestCheckResourceAttr("opennebula_template.template", "graphics.#", "1"),
resource.TestCheckResourceAttr("opennebula_template.template", "graphics.0.keymap", "en-us"),
resource.TestCheckResourceAttr("opennebula_template.template", "graphics.0.listen", "0.0.0.0"),
resource.TestCheckResourceAttr("opennebula_template.template", "graphics.0.type", "VNC"),
resource.TestCheckResourceAttr("opennebula_template.template", "os.#", "1"),
resource.TestCheckResourceAttr("opennebula_template.template", "os.0.arch", "x86_64"),
resource.TestCheckResourceAttr("opennebula_template.template", "os.0.boot", ""),
resource.TestCheckResourceAttr("opennebula_template.template", "context.%", "2"),
resource.TestCheckResourceAttr("opennebula_template.template", "context.dns_hostname", "yes"),
resource.TestCheckResourceAttr("opennebula_template.template", "context.NETWORK", "YES"),
resource.TestCheckResourceAttr("opennebula_template.template", "cpumodel.#", "1"),
resource.TestCheckResourceAttr("opennebula_template.template", "cpumodel.0.model", "host-passthrough"),
resource.TestCheckResourceAttr("opennebula_template.template", "tags.%", "2"),
Expand All @@ -88,20 +103,30 @@ func TestAccTemplate(t *testing.T) {
}),
),
},
{
ResourceName: "opennebula_template.template",
ImportState: true,
ImportStateVerify: true,
},
{
Config: testAccTemplateConfigUpdate,
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("opennebula_template.template", "name", "terratplupdate"),
resource.TestCheckResourceAttr("opennebula_template.template", "permissions", "642"),
resource.TestCheckResourceAttr("opennebula_template.template", "group", "oneadmin"),
resource.TestCheckResourceAttr("opennebula_template.template", "cpu", "1"),
resource.TestCheckResourceAttr("opennebula_template.template", "vcpu", "1"),
resource.TestCheckResourceAttr("opennebula_template.template", "memory", "768"),
resource.TestCheckResourceAttr("opennebula_template.template", "graphics.#", "1"),
resource.TestCheckResourceAttr("opennebula_template.template", "graphics.0.keymap", "en-us"),
resource.TestCheckResourceAttr("opennebula_template.template", "graphics.0.listen", "0.0.0.0"),
resource.TestCheckResourceAttr("opennebula_template.template", "graphics.0.type", "VNC"),
resource.TestCheckResourceAttr("opennebula_template.template", "os.#", "1"),
resource.TestCheckResourceAttr("opennebula_template.template", "os.0.arch", "x86_64"),
resource.TestCheckResourceAttr("opennebula_template.template", "os.0.boot", ""),
resource.TestCheckResourceAttr("opennebula_template.template", "context.%", "2"),
resource.TestCheckResourceAttr("opennebula_template.template", "context.dns_hostname", "yes"),
resource.TestCheckResourceAttr("opennebula_template.template", "context.NETWORK", "YES"),
resource.TestCheckResourceAttr("opennebula_template.template", "tags.%", "3"),
resource.TestCheckResourceAttr("opennebula_template.template", "tags.env", "dev"),
resource.TestCheckResourceAttr("opennebula_template.template", "tags.customer", "test"),
Expand All @@ -124,20 +149,30 @@ func TestAccTemplate(t *testing.T) {
}),
),
},
{
ResourceName: "opennebula_template.template",
ImportState: true,
ImportStateVerify: true,
},
{
Config: testAccTemplateConfigDelete,
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("opennebula_template.template", "name", "terratplupdate"),
resource.TestCheckResourceAttr("opennebula_template.template", "permissions", "642"),
resource.TestCheckResourceAttr("opennebula_template.template", "group", "oneadmin"),
resource.TestCheckResourceAttr("opennebula_template.template", "cpu", "1"),
resource.TestCheckResourceAttr("opennebula_template.template", "vcpu", "1"),
resource.TestCheckResourceAttr("opennebula_template.template", "memory", "768"),
resource.TestCheckResourceAttr("opennebula_template.template", "graphics.#", "1"),
resource.TestCheckResourceAttr("opennebula_template.template", "graphics.0.keymap", "en-us"),
resource.TestCheckResourceAttr("opennebula_template.template", "graphics.0.listen", "0.0.0.0"),
resource.TestCheckResourceAttr("opennebula_template.template", "graphics.0.type", "VNC"),
resource.TestCheckResourceAttr("opennebula_template.template", "os.#", "1"),
resource.TestCheckResourceAttr("opennebula_template.template", "os.0.arch", "x86_64"),
resource.TestCheckResourceAttr("opennebula_template.template", "os.0.boot", ""),
resource.TestCheckResourceAttr("opennebula_template.template", "context.%", "2"),
resource.TestCheckResourceAttr("opennebula_template.template", "context.dns_hostname", "yes"),
resource.TestCheckResourceAttr("opennebula_template.template", "context.NETWORK", "YES"),
resource.TestCheckResourceAttr("opennebula_template.template", "tags.%", "2"),
resource.TestCheckResourceAttr("opennebula_template.template", "tags.env", "dev"),
resource.TestCheckResourceAttr("opennebula_template.template", "tags.customer", "test"),
Expand All @@ -154,6 +189,11 @@ func TestAccTemplate(t *testing.T) {
}),
),
},
{
ResourceName: "opennebula_template.template",
ImportState: true,
ImportStateVerify: true,
},
},
})
}
Expand Down Expand Up @@ -233,7 +273,7 @@ resource "opennebula_template" "template" {

context = {
dns_hostname = "yes"
network = "YES"
NETWORK = "YES"
Copy link
Collaborator

@treywelsh treywelsh Oct 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Why did you change the case ? is it for test purposes ?
Not a big deal but this just add a bit of noise for this PR.

Copy link
Author

Choose a reason for hiding this comment

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

In the file opennebula/resource_opennebula_virtual_machine_test.go, there are a lot of resources with the key context. All these context-definitions have uppercase-keys, so it was easier to update this file opennebula/resource_opennebula_template_test.go. This is also an example of conflicts between uppercase-text (Opennebula) and lowercase-text (Terraform).

}

graphics {
Expand Down Expand Up @@ -273,7 +313,7 @@ resource "opennebula_template" "template" {

context = {
dns_hostname = "yes"
network = "YES"
NETWORK = "YES"
}

graphics {
Expand Down Expand Up @@ -319,7 +359,7 @@ resource "opennebula_template" "template" {

context = {
dns_hostname = "yes"
network = "YES"
NETWORK = "YES"
}

graphics {
Expand Down Expand Up @@ -356,7 +396,7 @@ resource "opennebula_template" "template" {

context = {
dns_hostname = "yes"
network = "YES"
NETWORK = "YES"
}

graphics {
Expand Down
2 changes: 1 addition & 1 deletion opennebula/resource_opennebula_virtual_machine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2028,7 +2028,7 @@ resource "opennebula_template" "template" {

context = {
dns_hostname = "yes"
network = "YES"
NETWORK = "YES"
}

graphics {
Expand Down
2 changes: 1 addition & 1 deletion opennebula/resource_opennebula_virtual_router_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ resource "opennebula_virtual_router_instance_template" "test" {

context = {
dns_hostname = "yes"
network = "YES"
NETWORK = "YES"
}

graphics {
Expand Down