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

add missing azure services #123

Merged
merged 1 commit into from
Mar 23, 2021
Merged

Conversation

xp-1000
Copy link
Contributor

@xp-1000 xp-1000 commented Mar 19, 2021

hello

Honestly, I don't understand why the azure integration does not allow to configure all services like other cloud integrations (gcp, aws) do especially since the underlying API accept it:

{"appId":null,"azureEnvironment":"AZURE","created":1613489793389,"creator":"xxx","enabled":true,"id":"xxx","lastUpdated":1613489793389,"lastUpdatedBy":"xxxEAE","name":"test-azure","namedToken":"test-azure","pollRate":300000,"secretKey":null,"services":[],"subscriptions":["yyy"],"syncGuestOsNamespaces":false,"tenantId":"zzz","type":"Azure"}

this example payload passing empty services array [] will enable all services in contrast of specifying a list of services which will only select those obviously.

Digging in git history I understand that it is a limitation from the terraform provider which make the services argument mandatory with at least one element:
splunk-terraform/terraform-provider-signalfx@0353e12#diff-aa526560492dd3432307ab965443674778bce1ce10e7c452f41975599b2716f6

However it seems to be a desired breaking change as changelog indicates: https://github.com/hashicorp/terraform-provider-signalfx/blob/master/CHANGELOG.md#4190-april-13-2020

Even if I would prefer to have the ability to configure all services easily without to know them like for aws and gcp there was probably a good reason to that but while I don't know/understand it I prefer to avoid any undesired side affect.

So I updated the azure services list to, at least, be able to configure all of them from the datasource given that it seems to be the only way even if it is not really future proof and will probably show similar problem when you will add new services in your integration.

I tested the terraform provider with this change and it works fine:

      ~ services                 = [
          + "microsoft.eventgrid/domains",
          + "microsoft.eventgrid/eventsubscriptions",
          + "microsoft.eventgrid/extensiontopics",
          + "microsoft.eventgrid/systemtopics",
          + "microsoft.eventgrid/topics",
          + "microsoft.maps/accounts",
          + "microsoft.network/azurefirewalls",
          + "microsoft.network/frontdoors",
          + "microsoft.network/networkinterfaces",
            # (59 unchanged elements hidden)
        ]

I am not sure how should I process to update the provider, should I wait for a tag here and create a Pull Request to bump the lib version used in go.mod here : https://github.com/splunk-terraform/terraform-provider-signalfx ? or simply create an issue ?

thanks

@xp-1000
Copy link
Contributor Author

xp-1000 commented Mar 23, 2021

cc @keitwb and @jrcamp (may be should you add relevant maintainers at Signalfx in default reviewers ?)

@keitwb
Copy link
Contributor

keitwb commented Mar 23, 2021

Hi @xp-1000, I've asked somebody who is more familiar with the Azure integration API to take a look.

@keitwb keitwb merged commit a5be1cb into signalfx:master Mar 23, 2021
@keitwb
Copy link
Contributor

keitwb commented Mar 23, 2021

Ok, so yes I'm not entirely sure why the provider requires at least one but for now just make a PR in the terraform repo to update this repo in go.mod. I made a tag v1.7.17 here to use. Being explicit with the service list in the provider I think is acceptable for now as it reduces uncertainty around what should be monitored or not.

@xp-1000
Copy link
Contributor Author

xp-1000 commented Mar 25, 2021

thanks @keitwb

Being explicit with the service list in the provider I think is acceptable for now as it reduces uncertainty around what should be monitored or not.

I agree even if it would be better if someone from Signalfx can update the terraform provider when you add new Azure service to your integration.

In my knowledge, these new services were not announced (e.g. in the "What is new" webui) so it is difficult to trigger the change from our side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants