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.

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

datadog_monitor tags overwritten #9375

Closed
nyanshak opened this issue Oct 14, 2016 · 7 comments
Closed

datadog_monitor tags overwritten #9375

nyanshak opened this issue Oct 14, 2016 · 7 comments

Comments

@nyanshak
Copy link
Contributor

Terraform Version

Terraform v0.7.5

Affected Resource(s)

  • datadog_monitor

The tags for Datadog monitors are for filtering, and are implemented in terraform as a map, but I think it should be a list instead to better match Datadog's API.

builtin/providers/datadog/resource_datadog_monitor.go:

tags := make(map[string]string)
for _, s := range m.Tags {
tag := strings.Split(s, ":")
tags[tag[0]] = tag[1]
}

These lines split the tag on colon and add to a map, but this behavior is not correct if you have (for example):

  • example_tag:value1
  • example_tag:value2

This is valid in Datadog, but only the second value will be added to the terraform resource, since it's a map instead of an array.

This can result in terraform overwriting tags that should be tracked.

ojongerius added a commit to atlassian/terraform that referenced this issue Dec 7, 2016
… map.

Tags are allowed to be but not restricted to, key value pairs (ie: foo:bar)
but are esssentially strings. This changes allows using, and mixing of tags with
form "foo" and "foo:bar". It also allows using duplicate keys like "foo:bar" and "foo:baz".
stack72 pushed a commit that referenced this issue Dec 7, 2016
…0570)

* provider/datadog #9375: Refactor tags to a list instead of a map.
Tags are allowed to be but not restricted to, key value pairs (ie: foo:bar)
but are esssentially strings. This changes allows using, and mixing of tags with
form "foo" and "foo:bar". It also allows using duplicate keys like "foo:bar" and "foo:baz".

* provider/datadog update import test.
ojongerius added a commit to atlassian/terraform that referenced this issue Dec 8, 2016
…dentials

* upstream/master: (79 commits)
  update CHANGELOG
  Update panicwrap to pass through all interrupt signals
  Gracefully stops on SIGTERM
  website: update website for conditionals
  vendor: update HIL with conditionals
  Keep a consistent provider order.
  Update CHANGELOG.md
  provider/aws: Forces the api gateway domain name certificates to recreate the resource (hashicorp#10588)
  Update CHANGELOG.md
  provider/aws: FIxed the api_gw_domain_name replace operation (hashicorp#10179)
  Fixed note formatting
  Explicitly say `count` is not supported by modules (hashicorp#10553)
  docs/aws: Fix the discrepencies of the emr_cluster documentation (hashicorp#10578)
  Update CHANGELOG.md
  Service role is not updated on AWS for a CodeDeploy deployment group (hashicorp#9866)
  Update CHANGELOG.md
  provider/datadog hashicorp#9375: Refactor tags to a list instead of a map. (hashicorp#10570)
  Update the Vagrantfile to resolve package update/installation issue. (hashicorp#9783)
  docs/aws: Add iam_server_certificate data source to nav bar (hashicorp#10576)
  Update CHANGELOG.md
  ...
@ojongerius
Copy link
Contributor

ojongerius commented Dec 12, 2016

@stack72 you've merged #10570. Users will get errors if they do not update their configs. What do you think is the best way forward: just updating the changelog with a warning, or should we give a warning when the config has a map, instead of the list we expect?
When this provider was external I've done similar things with breaking changes as a courtesy to users. Not sure what is common in Terraform.

@stack72
Copy link
Contributor

stack72 commented Dec 12, 2016

Hi @ojongerius

I think we should note this in the incompatibilities section of the changelog. The reason I merged this was because we are about to go to 0.8 and it seemed like it was the right place to make the change (major version)

Do you think something like this:

provider/datadog: datadog_monitor should be a map now and no longer a list

?

pielu pushed a commit to pielu/terraform that referenced this issue Dec 20, 2016
* aws/feature/r-instance-net-iface-id: (74 commits)
  - Properly exercise network_interface_id from AWS SDK - Update Terraform’s documentation
  Update CHANGELOG.md
  provider/aws: Forces the api gateway domain name certificates to recreate the resource (hashicorp#10588)
  Update CHANGELOG.md
  provider/aws: FIxed the api_gw_domain_name replace operation (hashicorp#10179)
  Fixed note formatting
  Explicitly say `count` is not supported by modules (hashicorp#10553)
  docs/aws: Fix the discrepencies of the emr_cluster documentation (hashicorp#10578)
  Update CHANGELOG.md
  Service role is not updated on AWS for a CodeDeploy deployment group (hashicorp#9866)
  Update CHANGELOG.md
  provider/datadog hashicorp#9375: Refactor tags to a list instead of a map. (hashicorp#10570)
  Update the Vagrantfile to resolve package update/installation issue. (hashicorp#9783)
  docs/aws: Add iam_server_certificate data source to nav bar (hashicorp#10576)
  Update CHANGELOG.md
  feat/aws: add iam_server_certificate data source (hashicorp#10558)
  provider/azurerm: arm_virtual_machine panic fix
  Update .travis.yml
  provider/aws: Improved the documentation for EMR Cluster (hashicorp#10563)
  provider/azurerm: Do not pass an empty string of license_type to AMR VMs (hashicorp#10564)
  ...

# Conflicts:
#	builtin/providers/aws/resource_aws_instance.go
@jaygorrell
Copy link

@stack72 That's backwards, but otherwise that should be fine.

@stack72
Copy link
Contributor

stack72 commented Mar 2, 2017

Hi

Do we still think this is an error? It's been 2 months since this was reported and released

Paul

@nyanshak nyanshak closed this as completed Mar 2, 2017
@nyanshak nyanshak reopened this Mar 2, 2017
@nyanshak
Copy link
Contributor Author

nyanshak commented Mar 2, 2017

This seems to be properly addressed by 81799b4. Closing

@nyanshak nyanshak closed this as completed Mar 2, 2017
@ojongerius
Copy link
Contributor

🎉

@ghost
Copy link

ghost commented Apr 16, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants