Skip to content
This repository has been archived by the owner on Jul 26, 2021. It is now read-only.

add tags in resources vpc, instance, ipadress, template and vpc #16

Merged
merged 2 commits into from
Oct 13, 2017
Merged

add tags in resources vpc, instance, ipadress, template and vpc #16

merged 2 commits into from
Oct 13, 2017

Conversation

ArthurHlt
Copy link
Contributor

User could tags only network, I've added tags parameter to more cloudstack resource.

Actually we can tags everything except security group but api docs about tags ask for resourceType and their is no simple way to know what can be this values.

I've found some of them and added the tags parameter.

There is a refactoring to test tags in resource.

Finally, I couldn't run acceptance tests only because I'm not sure what should inside env vars and how it could affect my cloudstack.
Maybe just some comments could help

Copy link
Member

@svanharmelen svanharmelen left a comment

Choose a reason for hiding this comment

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

I have a few small comments, but looks good overall.

Guess you didn't test this with all resource you changed right (as the disk resource doesn't seem to have tags in it's schema now)? Without it being properly tested I cannot merge this PR. So please test all resources your changed.

Additionally I will see if I can run the acceptance tests once you have updated and tested. Thanks!

// Put tags if necessary
err = setTags(cs, d, "Volume")
if err != nil {
return fmt.Errorf("Error setting tags on the new instance %s: %s", name, err)
Copy link
Member

Choose a reason for hiding this comment

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

Should be on the new disk I guess

// Update tags if they have changed
err := updateTags(cs, d, "Volume")
if err != nil {
return fmt.Errorf("Error setting tags on the disk %s: %s", name, err)
Copy link
Member

Choose a reason for hiding this comment

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

Should be Error updating tags

// Update tags if they have changed
err := updateTags(cs, d, "userVm")
if err != nil {
return fmt.Errorf("Error setting tags on the instance %s: %s", name, err)
Copy link
Member

Choose a reason for hiding this comment

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

Should be Error updating tags

@@ -116,6 +116,13 @@ func resourceCloudStackDiskCreate(d *schema.ResourceData, meta interface{}) erro

Copy link
Member

Choose a reason for hiding this comment

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

I think you forgot to add "tags": tagsSchema() to the disk schema?

@ArthurHlt
Copy link
Contributor Author

Thank you @svanharmelen for the review and sorry for the long delay.
This is now fixed, we have tested these new tags and everything works, acceptance test can be ran.

@svanharmelen
Copy link
Member

Thanks @ArthurHlt! LGTM!

@svanharmelen svanharmelen merged commit 47459f8 into xanzy:master Oct 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants