terraform-provider-newrelic
terraform-provider-newrelic copied to clipboard
newrelic
feat(Dashboard json): Add parameter for query accounts
When a dashboard needs to exist for more than the limit of five accounts that can be queried at a time, it has to be duplicated, except for the accountIDs. The only way to do that today with the newrelic_one_dashboard_json resource is to template the underlying json.
This in turn completely breaks the workflow of "Edit dashboard in UI, export json into terraform", because a post-processing step is needed to insert all the template expressions into the json document.
This change aims to fix that by adding a new data_accounts parameter into the newrelic_one_dashboard_json resource that if set will replace all the accountIDs in the json document.
Type of change
Please delete options that are not relevant.
- [ ] Bug fix (non-breaking change which fixes an issue)
- [X] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update
Checklist:
Please delete options that are not relevant.
How to test this change?
Please describe how to test your changes. Include any relevant steps in the UI, HCL file(s), commands, etc
- Create a dashboard in the UI
- Export the JSON
- Use the
newrelic_one_dashboard_jsonresource with thedata_accountsparameter set to create a dashboard - Inspect the created dashboard and verify in its json that all the
accountIDsare what what indata_accounts, regardless of what the original JSON contained
Codecov Report
Merging #2454 (f2ea80f) into main (f9aa096) will increase coverage by
0.04%. Report is 2 commits behind head on main. The diff coverage is53.12%.
@@ Coverage Diff @@
## main #2454 +/- ##
==========================================
+ Coverage 33.87% 33.92% +0.04%
==========================================
Files 91 91
Lines 24884 24948 +64
==========================================
+ Hits 8429 8463 +34
- Misses 16300 16321 +21
- Partials 155 164 +9
| Files | Coverage Ξ | |
|---|---|---|
| newrelic/resource_newrelic_one_dashboard_json.go | 29.50% <100.00%> (+3.64%) |
:arrow_up: |
| newrelic/structures_newrelic_one_dashboard_json.go | 40.32% <45.45%> (+40.32%) |
:arrow_up: |
:mega: Weβre building smart automated test selection to slash your CI/CD build times. Learn more
Hi @alvaroaleman - thank you for the contribution! Sorry, we've been able to get this after quite some time as the team has lately been occupied with multiple priority items. I have a few things to mention -
-
For me to be able to communicate better with the team about this enhancement and to get a review, we'd love if you can elaborate further on the issue you've described in the description of this PR, which you'd like to fix via the code changes in the PR. Can you please give us a good example, what you're experiencing and how overriding account IDs in the JSON configuration would help solve the problem? This would help us take the review further. Thanks!
-
A few preliminary review comments -
a) Great work on the addition of a test case to newrelic/structures_newrelic_one_dashboard_json.go :) it would also be great to have a Terraform Acceptance Test when the attribute data_accounts is added to the Terraform Configuration - please refer to newrelic/resource_newrelic_one_dashboard_json_test.go for samples of acceptance tests.
b) A few lint failures are currently being thrown by the lint test running in the CI. You would find the cause of this shown in the changes tab of this PR. Would you be able to take a look and correct them accordingly? Also, as a tip, running make lint before you make a commit would make sure a lint test works fine with your code changes.
c) It would also be helpful if you can pull the latest changes from main onto your branch (I'll try doing that too, but in case you still see the message This branch is out-of-date with the base branch, please pull latest changes.)
Looking forward to your response (and more changes) π thanks!
@alvaroaleman I've tested this quite a few times, and it seems to work as intended (the accountIds of each NRQL query are overridden by the account IDs specified in the data_accounts argument). The code looks good, with apt error handling at all required junctures :)
However, since this is a request for an 'enhancement', we would need to review this within the engineering team further, just to make sure everything's fine to take this forward. In the meanwhile, as specified in my previous comment - it would be really helpful if you can give us a descriptive example of how these changes would help address the usecase you've mentioned (just so a descriptive, working example would help me take this forward to the team for an analysis of this requirement, and a review). I'll try investigating into this further too, in the meanwhile, to identify an apt usecase, but an example you can add would help me do this.
I'll get this reviewed further in the next one week; considering current team priorities, we'd need a bit of time to bring this up for review - would appreciate your patience with this :) thanks!
For me to be able to communicate better with the team about this enhancement and to get a review, we'd love if you can elaborate further on the issue you've described in the description of this PR, which you'd like to fix via the code changes in the PR. Can you please give us a good example, what you're experiencing and how overriding account IDs in the JSON configuration would help solve the problem? This would help us take the review further. T
@pranav-new-relic Let me try to give a practical example to illustrate the issue:
variables {
dashboardAccountToDataAccount = {
"dashboardaccountA": ["dataaccount1", "dataaccount2","dataaccount3", "dataaccount4", "dataaccount5"]
"dashboardaccountB": ["dataaccount6", "dataaccount7","dataaccount8", "dataaccount9", "dataaccount10"]
}
}
# Todays state, makes it impossible to copy json from the UI as a template expresison has to be inserted for each widget
resource "newrelic_one_dashboard_json" "my-dashboard" {
for_each = var.dashboardAccountToDataAccount
json = templatefile("some_file", {DATA_ACCOUNT_IDS: each.value})
account_id: each.key
}
# Desired state, implemented by this change, allows to copy the JSON from the UI without any further adjustments needed
resource "newrelic_one_dashboard_json" "my-dashboard" {
for_each = var.dashboardAccountToDataAccount
json = file("some_file")
data_accounts: each.value
account_id: each.key
}
(I just handwrote this, its probably has some syntax issues but I think it illustrates the problem). Please let me know if this is sufficient. I will look into an acceptance test and linting in the meanwhile.
Hey @alvaroaleman - this illustration looks great, thanks for sharing! However, we're finding it a bit difficult to understand the exact limitation (when the limit of five accounts is exceeded) via the variables and the template file.
Would you mind attaching an example that we would be able to reproduce (the JSON with a few sample widgets, the template file that is written and the variables)? This would help us plan the impact of these changes accordingly too, as the team is planning to check if we've received enhancement requests similar to this in the past. Thank you!
@pranav-new-relic you can literally use the json file from the testdata and insert the template expressions, I did it for you below (notice ${join(", ", ACCOUNT_IDS)}). Most dashboards have more widgets than the test example. As mentioned, this breaks the workflow of
- Edit dashboard in UI
- Export the JSON
- Manually edit the json, replace all the accountIDs in each variable (which are multiple elements, as each widgets quries five accounts) with a template exppression as seen below
{
"description": "The Dashboard",
"name": "Dashboard",
"pages": [
{
"description": "Overview",
"name": "Overview",
"widgets": [
{
"layout": {
"column": 1,
"height": 3,
"row": 1,
"width": 6
},
"linkedEntityGuids": null,
"rawConfiguration": {
"nrqlQueries": [
{
"accountIds": [
${join(", ", ACCOUNT_IDS)}
],
"query": "SELECT foo FROM Metric TIMESERIES"
},
{
"accountIds": [
123
],
"query": "SELECT bar FROM Metric TIMESERIES"
}
]
},
"title": "title",
"visualization": {
"id": "viz.table"
}
},
{
"layout": {
"column": 7,
"height": 3,
"row": 1,
"width": 6
},
"rawConfiguration": {
"nrqlQueries": [
{
"accountIds": [
${join(", ", ACCOUNT_IDS)}
],
"query": "SELECT foo FROM Metric TIMESERIES"
},
{
"accountIds": [
123
],
"query": "SELECT bar FROM Metric TIMESERIES"
}
]
},
"title": "Other Title",
"visualization": {
"id": "viz.table"
}
}
]
}
],
"permissions": "PUBLIC_READ_WRITE",
"variables": [
{
"defaultValues": [
{
"value": {
"string": "*"
}
}
],
"isMultiSelection": true,
"items": null,
"name": "env",
"nrqlQuery": {
"accountIds": [
${join(", ", ACCOUNT_IDS)}
],
"query": "SELECT uniques(`deployment.environment`) FROM Metric"
},
"replacementStrategy": "DEFAULT",
"title": null,
"type": "NRQL"
},
{
"defaultValues": [
{
"value": {
"string": "*"
}
}
],
"isMultiSelection": true,
"items": null,
"name": "sync_env",
"nrqlQuery": {
"accountIds": [
${join(", ", ACCOUNT_IDS)}
],
"query": "SELECT uniques(`sync_env`) FROM Metric"
},
"replacementStrategy": "DEFAULT",
"title": null,
"type": "NRQL"
}
]
}
Also, as a tip, running make lint before you make a commit would make sure a lint test works fine with your code changes.
This unfortunately does not work:
$ make lint
tools.go:7:2: import "github.com/bflad/tfproviderlint/cmd/tfproviderlint" is a program, not an importable package
=== terraform-provider-newrelic === [ tools ]: Installing tools required by the project...
package github.com/newrelic/terraform-provider-newrelic/v2/tools: build constraints exclude all Go files in /Users/aaleman/git/go/src/github.com/newrelic/terraform-provider-newrelic/tools
make: *** [tools] Error 1
I've added an acceptance test, please let me know what you think.
Thanks for your prompt responses and code corrections, @alvaroaleman. The changes look good to me. As a practice, when we take contributions, we make sure we invest time in getting the submission reviewed multi-fold (with more eyes from the team) and testing the change out thoroughly to ensure this does not result in a breaking change. Though most of your code addition is new and wouldn't cause a breaking change :) we'd be extra cautious while pushing changes like these out. Given a few immediate priorities of the team, we shall be able to push this process a bit at a time, and shall be done with this by early/mid-next week. We'd reach out to you further if my colleagues find the need to have anything else in the code to be addressed, but in the meanwhile, would appreciate your patience with this.
On that note, thank you for your submission - this is a really wholesome PR with good error handling and tests, as I must have mentioned previously π
Hi @alvaroaleman, an elementary question we'd like to ask, as we were taking this PR through a review:
When a dashboard needs to exist for more than the limit of five accounts that can be queried at a time, it has to be
This looks like this might be something specific to your pricing model, as we find that the "five account" limit is not evident in the documentation at this point, we'll need to figure out why. However, we'd like to know how you'd observed this behaviour - did you find this mentioned somewhere in the docs, or when you try adding more than five accounts, do you see an error (and if so, would you mind sharing what you see)? This would help us make sure this isn't specific only to you :) thanks!
did you find this mentioned somewhere in the docs
Never mind, got this answered via an internal channel by John Hwang. Thank you.
Hi @alvaroaleman thank you for your contribution! I'm seeing some test failures -TestAccNewRelicOneDashboardJson_Create and TestAccNewRelicOneDashboardJson_CreateWithDataAccounts. Can you take a look at these?
@mbazhlekova where did you see those failures? GitHub showed all checks as success. Is the fix for the incorrect fmt string enough?
@alvaroaleman I checked out your branch and ran the test suite locally. Unfortunately the repo isn't setup to run integration tests against forks.
Ah, thanks for doing that. Does it pass now? If not, would you be able to paste the error?
@alvaroaleman Sure, here are the failures I'm seeing:
=== FAIL: newrelic TestAccNewRelicOneDashboardJson_CreateWithDataAccounts (7.05s) provider_test.go:237: testacc cleanup of 0 applications complete resource_newrelic_one_dashboard_json_test.go:48: Step 1/1 error: Error running pre-apply refresh: exit status 1
Error: Incorrect attribute value type
on terraform_plugin_test.tf line 75, in resource "newrelic_one_dashboard_json" "bar":
75: data_accounts = ["%!s(MISSING)"]
Inappropriate value for attribute "data_accounts": a number is required.
=== FAIL: newrelic TestAccNewRelicOneDashboardJson_Create (7.68s) provider_test.go:232: deleted application 599028582 (1/6) provider_test.go:237: testacc cleanup of 1 applications complete resource_newrelic_one_dashboard_json_test.go:23: Step 1/2 error: Error running apply: exit status 1
Error: query error; Input: [Variable with name 'variableNRQL' has the following errors:: [Account ids can't be an empty list]]
with newrelic_one_dashboard_json.bar,
on terraform_plugin_test.tf line 3, in resource "newrelic_one_dashboard_json" "bar":
3: resource "newrelic_one_dashboard_json" "bar" {
DONE 2 tests, 2 failures in 8.661s
Hi @mbazhlekova - thank you for identifying this; sorry, I missed this important catch as I was juggling a couple of things π
I also just ran a make test-integration locally and also find that the two tests (linked to newrelic_one_dashboard_json) you've mentioned above are failing
@mbazhlekova thanks for your feedback. I've pushed a new revision which I think fixes the issues you are seeing. Could you please verify? Thank you in advance!
@alvaroaleman Sure, I just ran the tests again. I'm seeing this failure:
=== FAIL: newrelic TestAccNewRelicOneDashboardJson_CreateWithDataAccounts (0.82s) resource_newrelic_one_dashboard_json_test.go:48: Step 1/1 error: Error running apply: exit status 1
Error: query error; Invalid widget input: nrql queries can not provide both 'accountId' and 'accountIds' at the same time
with newrelic_one_dashboard_json.bar,
on terraform_plugin_test.tf line 3, in resource "newrelic_one_dashboard_json" "bar":
3: resource "newrelic_one_dashboard_json" "bar" {
Hi @alvaroaleman - I spent quite some time trying to understand why the above error is seen with the acceptance test you've added, and here are my observations - please take a look some time when free π
1. accountId vs accountIds
The error message shown in the above acceptance test is pretty descriptive; and indicates both accountId and accountIds are being added to the request sent to the API, and hence, the API responds with this error.
This is because in rawConfiguration > nrqlQueries, apparently, both accountId and accountIds are valid attribute names. accountId is expected to take a single account ID, while accountIds is expected to take a list of account IDs, similar to how you're populating data_accounts in the code.
In the test case you're using, the JSON file that already exists has accountId and not accountIds; since your code addition only helps replace/add accountIds, the above error is seen.
https://github.com/alvaroaleman/terraform-provider-newrelic/blob/add-data-accounts/newrelic/resource_newrelic_one_dashboard_json_test.go#L172-L177
Though we can have a different test that only has accountIds in the JSON, I think this is a good edge case that has been unearthed because of the above error. I think you might want to modify the code to add/replace the attribute accountIds with values in data_accounts, and also discard accountId from the rawConfiguration > nrqlQueries, if any. This should mitigate the first issue.
https://github.com/alvaroaleman/terraform-provider-newrelic/blob/f2ea80fa2297e30d4b7cb81e7f24cae3232842c2/newrelic/structures_newrelic_one_dashboard_json.go#L57-L64
2. The Validation Used In The Test Case
I find that a validation function you've defined in the test goes like
https://github.com/alvaroaleman/terraform-provider-newrelic/blob/add-data-accounts/newrelic/resource_newrelic_one_dashboard_json_test.go#L62-L64
which basically searches for all instances of the original account ID in the string version of the JSON and throws an error if found. This is what I experienced when to counter case (1) above, I replaced accountIds with accountId and then run the test, but it always failed with this validation error.
Unfortunately, this function will always fail, because of the way a dashboard that is created is read from NerdGraph (the API) by Terraform. The NerdGraph query that is called by the provider doesn't exactly return the JSON version of the dashboard, but a response with details of the entity whose GUID is that of the dashboard (this is how the read function works across most resources), which is why it would contain a few fields still containing the original account ID with which the dashboard was created.
I'd love to give an example here, but it would be too lengthy - you can try running the following query on NerdGraph with the account ID the dashboard was created from (to obtain accurate results, please make sure that you use valid, existent account IDs in the data_accounts argument while creating the dashboard; these can be subaccounts of your account too, but using invalid entries such as 123456789 would cause discrepancies in the NerdGraph result returned).
https://github.com/newrelic/newrelic-client-go/blob/a8b0fd12945956e1b00cb093ba7425443345cf1d/pkg/dashboards/dashboard.go#L38
Upon querying, you'd be able to see that the response would still contain the account ID with which the dashboard was created; you might want to iterate through these widgets and get the accountIds in nrqlQueries in rawConfiguration, and similarly with variables, or any simpler approach you can think of π
3. The Mock Account ID 123456789 Used In The Test
As I mention above, the response of the above query appears to be out of alignment with what is expected when mock account IDs are given - I experienced this when I created a dashboard with 123456789 in data_accounts and then, tried to query it from NerdGraph. We'd expect that while creating a dashboard, an error would need to be thrown, stating that the account ID is invalid, which isn't the case now. We'll take this to the right team to get fixed; but at the moment, the response returned by the query is what affects the test case, and hence, we'd need to work this around by using valid account IDs in data_accounts.
A way through this could be to use the environment variable NEW_RELIC_SUBACCOUNT_ID (if you haven't set this already in your environment, you can, by adding a valid subaccount against NEW_RELIC_SUBACCOUNT_ID in your environment), and then using it as the input to the test case. To fetch the subaccount ID, you can use this as an example:
https://github.com/newrelic/terraform-provider-newrelic/blob/660a2b00ebd18c454715d11736ebec6500f9db64/newrelic/resource_newrelic_cloud_aws_integrations_test.go#L22
I think this could be an optimal method, so we can test it directly on our end too without any changes, with the NEW_RELIC_SUBACCOUNT_ID we have.
Sounds like a lot of changes - please take your time to read through this - shall be awaiting your changes. Thanks!
Thanks for your feedback! I am out of office this week, will come back to it next.