Add datasource cloudstack_template #21

cryptobioz · 2017-11-16T07:24:25Z

This datasource allows you to retrieve a template according to filters you set.
There can be several versions for one template, the datasource will always returns the most recent one.

Parameters:

templatefilter is needed according to the Cloudstack API (https://cloudstack.apache.org/api/apidocs-4.9/apis/listTemplates.html).
filter is used to sort templates.

If you set a filter, the datasource will only returns templates where the field name has the value value (the value can be a regex).
To see the list of available options, check the Template struct of the Cloudstack client : https://godoc.org/github.com/xanzy/go-cloudstack/cloudstack#Template

Example:

data "cloudstack_template" "my_template" {
    templatefilter = "featured"
    filter {
        name = "name"
        value = "CentOS 7\\.1"
    }
    filter {
        name = "hypervisor"
        value = "KVM"
    }
}

Note:

At the moment, filters only work with string fields, you will not be able to set options like size, tags or bootable.

svanharmelen

Thanks for this one @cryptobioz! I do have a few comments we should look at first, but this will make a nice addition to the provider! 👍

svanharmelen · 2017-11-17T10:55:18Z

cloudstack/data_source_cloudstack_template.go

+	for k, t := range templates {
+		created, err := time.Parse("2006-01-02T15:04:05-0700", t.Created)
+		if err != nil {
+			panic(err)


Please do not panic here. You should never panic, but instead just return the error. So mostRecentTemplate should then return (*cloudstack.Template, error)...

svanharmelen · 2017-11-17T10:57:22Z

cloudstack/data_source_cloudstack_template.go

+		Read: dataSourceCloudstackTemplateRead,
+		Schema: map[string]*schema.Schema{
+			"filter": dataSourceFiltersSchema(),
+			"templatefilter": {


In TF resources the schema attributes are usually written using snake case. So templatefilter should be template_filter instead.

svanharmelen · 2017-11-17T11:01:09Z

cloudstack/data_source_cloudstack_template.go

+				Type:     schema.TypeString,
+				Computed: true,
+			},
+			"displaytext": {


This should be display_text

svanharmelen · 2017-11-17T11:02:41Z

cloudstack/data_source_cloudstack_template.go

+
+func dataSourceCloudstackTemplateRead(d *schema.ResourceData, meta interface{}) error {
+	cs := meta.(*cloudstack.CloudStackClient)
+	ltp := cloudstack.ListTemplatesParams{}


nitting In other places within this provider we just call the param struct p instead of in this case ltp. And please insert a blank line between line 62 and 63 for readability 😉

svanharmelen · 2017-11-17T11:04:04Z

cloudstack/data_source_cloudstack_template.go

+	ltp := cloudstack.ListTemplatesParams{}
+	ltp.SetListall(true)
+	ltp.SetTemplatefilter(d.Get("templatefilter").(string))
+	csTemplates, err := cs.Template.ListTemplates(&ltp)


nitting Please insert a blank line between line 64 and 65 for readability

svanharmelen · 2017-11-17T11:08:30Z

cloudstack/data_source_cloudstack_common_schema.go

+func dataSourceFiltersSchema() *schema.Schema {
+	return &schema.Schema{
+		Type:     schema.TypeSet,
+		Optional: true,


Why make this optional? I assume this call is useless without a filter right? So I suggest making this required instead.

svanharmelen · 2017-11-17T11:23:17Z

cloudstack/data_source_cloudstack_template.go

+}
+
+func templateDescriptionAttributes(d *schema.ResourceData, template *cloudstack.Template) error {
+	d.SetId(template.Id)


Is this needed for a data source? To set the ID?

You have to put an ID on a datasource. I chose to use the template ID like in the datasource aws_ami.

Ah, check... I wasn't sure a datasource also needed an ID. All clear then...

svanharmelen · 2017-11-17T11:29:51Z

cloudstack/data_source_cloudstack_template.go

+
+	for _, f := range filters.List() {
+		m := f.(map[string]interface{})
+		r := regexp.MustCompile(m["value"].(string))


regexp.MustCompile generates a panic if the regex cannot compile. As this is user input we do not know upfront, we cannot be sure it will not panic. So instead please use regexp.Compile and check (and return if needed) the error.

svanharmelen · 2017-11-17T11:31:56Z

cloudstack/data_source_cloudstack_template.go

+		m := f.(map[string]interface{})
+		r := regexp.MustCompile(m["value"].(string))
+		templateField := templateJSON[m["name"].(string)].(string)
+		if !r.Match([]byte(templateField)) {


Maybe r.MatchString is a better fit here?

svanharmelen · 2017-11-17T11:41:05Z

cloudstack/data_source_cloudstack_template.go

+	}
+
+	return templates[mrt]
+}


Another suggestion...

func mostRecentTemplate(templates []*cloudstack.Template) (*cloudstack.Template, error) { var latest *time.Time var template *cloudstack.Template for _, t := range templates { created, err := time.Parse("2006-01-02T15:04:05-0700", t.Created) if err != nil { return nil, err } if latest == nil || created.After(latest) { latest = created template = t } } return template, nil }

cryptobioz · 2017-11-17T14:06:19Z

Thanks for the review, I made the requested changes.

svanharmelen · 2017-11-17T16:42:32Z

There is still a small part that I would like to change/improve to make it a bit more idiomatic Go (using if..else is really discouraged in Go).

But overall it looks good now, so I'm going to merge this as is and then add a few small tweaks/fixes. Thanks again for the work!

svanharmelen · 2017-11-17T16:58:39Z

@cryptobioz see this commit for my additional changes: 1ca338f

One more thing I totally forgot about is the documentation or this new data source! Would/could you please open a new PR with some docs (doesn't have to be much) to go with this data source?

That would be awesome and is of course needed to make people aware of the fact that this data source now exists 😉 Thx!

cryptobioz · 2017-11-18T17:18:45Z

I'm sorry that you had to rewrite almost everything, however, it's very instructive.
I'll open a new PR to add documentation to this datasource.
Thanks for your time and for the merge!

svanharmelen · 2017-11-19T09:23:13Z

Don't be sorry! I only tweaked the style a bit, the logic is still all you 👍

cryptobioz added 2 commits November 14, 2017 08:21

Add datasource template

e1577da

Use filters

29b827a

svanharmelen suggested changes Nov 17, 2017

View reviewed changes

Fix

94e89ee

svanharmelen approved these changes Nov 17, 2017

View reviewed changes

svanharmelen merged commit 5b5a218 into xanzy:master Nov 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add datasource cloudstack_template #21

Add datasource cloudstack_template #21

cryptobioz commented Nov 16, 2017

svanharmelen left a comment

svanharmelen Nov 17, 2017

svanharmelen Nov 17, 2017

svanharmelen Nov 17, 2017

svanharmelen Nov 17, 2017

svanharmelen Nov 17, 2017

svanharmelen Nov 17, 2017

svanharmelen Nov 17, 2017

cryptobioz Nov 17, 2017

svanharmelen Nov 17, 2017

svanharmelen Nov 17, 2017

svanharmelen Nov 17, 2017

svanharmelen Nov 17, 2017

cryptobioz commented Nov 17, 2017

svanharmelen commented Nov 17, 2017

svanharmelen commented Nov 17, 2017

cryptobioz commented Nov 18, 2017

svanharmelen commented Nov 19, 2017

Add datasource cloudstack_template #21

Add datasource cloudstack_template #21

Conversation

cryptobioz commented Nov 16, 2017

Parameters:

Example:

Note:

svanharmelen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cryptobioz commented Nov 17, 2017

svanharmelen commented Nov 17, 2017

svanharmelen commented Nov 17, 2017

cryptobioz commented Nov 18, 2017

svanharmelen commented Nov 19, 2017