feat(Dashboard json): Add parameter for query accounts #2454

alvaroaleman · 2023-09-03T18:29:18Z

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)
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_json resource with the data_accounts parameter set to create a dashboard
Inspect the created dashboard and verify in its json that all the accountIDs are what what in data_accounts, regardless of what the original JSON contained

CLAassistant · 2023-09-03T18:29:24Z

All committers have signed the CLA.

codecov-commenter · 2023-09-21T05:27:52Z

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 is 53.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%)`	⬆️
newrelic/structures_newrelic_one_dashboard_json.go	`40.32% <45.45%> (+40.32%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pranav-new-relic · 2023-09-21T06:13:26Z

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 · 2023-09-22T16:54:09Z

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.

pranav-new-relic · 2023-09-22T17:58:43Z

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!

alvaroaleman · 2023-09-22T18:38:56Z

@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"
    }
  ]
}

alvaroaleman · 2023-09-22T21:12:57Z

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.

newrelic/structures_newrelic_one_dashboard_json_test.go

newrelic/resource_newrelic_one_dashboard_test.go

pranav-new-relic · 2023-09-25T16:37:21Z

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 😄

mbazhlekova · 2023-09-28T19:22:11Z

newrelic/resource_newrelic_one_dashboard_json_test.go

+							return fmt.Errorf("dashboard '%s' included original accountID %d", string(serialized), testAccountID)
+						}
+						if !bytes.Contains(serialized, []byte(dataAccountID)) {
+							return fmt.Errorf("dashboard '%s' did not include updated accountID $d", string(serialized), dataAccountID)


Suggested change

return fmt.Errorf("dashboard '%s' did not include updated accountID $d", string(serialized), dataAccountID)

return fmt.Errorf("dashboard '%s' did not include updated accountID %s", string(serialized), dataAccountID)

mbazhlekova · 2023-09-28T19:50:49Z

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?

alvaroaleman · 2023-09-28T21:38:03Z

@mbazhlekova where did you see those failures? GitHub showed all checks as success. Is the fix for the incorrect fmt string enough?

mbazhlekova · 2023-09-28T21:49:16Z

@alvaroaleman I checked out your branch and ran the test suite locally. Unfortunately the repo isn't setup to run integration tests against forks.

alvaroaleman · 2023-09-28T22:03:10Z

Ah, thanks for doing that. Does it pass now? If not, would you be able to paste the error?

mbazhlekova · 2023-09-29T14:43:52Z

@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

pranav-new-relic · 2023-09-29T15:40:17Z

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

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, expect 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.

alvaroaleman · 2023-09-29T16:15:40Z

@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!

mbazhlekova · 2023-10-02T16:27:46Z

@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" {

pranav-new-relic · 2023-10-10T17:10:40Z

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:

terraform-provider-newrelic/newrelic/resource_newrelic_cloud_aws_integrations_test.go

Line 22 in 660a2b0

    
           if subAccountIDExists := os.Getenv("NEW_RELIC_SUBACCOUNT_ID"); subAccountIDExists == "" {

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!

alvaroaleman · 2023-10-11T02:39:54Z

Thanks for your feedback! I am out of office this week, will come back to it next.

This comment was marked as outdated.

Sign in to view

alvaroaleman force-pushed the add-data-accounts branch from e8d7ded to 2fc97ef Compare September 22, 2023 21:11

pranav-new-relic reviewed Sep 25, 2023

View reviewed changes

newrelic/structures_newrelic_one_dashboard_json_test.go Outdated Show resolved Hide resolved

pranav-new-relic reviewed Sep 25, 2023

View reviewed changes

newrelic/resource_newrelic_one_dashboard_test.go Show resolved Hide resolved

alvaroaleman force-pushed the add-data-accounts branch from 2fc97ef to 4ea505e Compare September 25, 2023 14:18

This comment was marked as outdated.

Sign in to view

mbazhlekova suggested changes Sep 28, 2023

View reviewed changes

alvaroaleman force-pushed the add-data-accounts branch from 4ea505e to cf8c5a4 Compare September 28, 2023 21:37

alvaroaleman force-pushed the add-data-accounts branch from cf8c5a4 to f2ea80f Compare September 29, 2023 16:15

alvaroaleman requested a review from mbazhlekova September 29, 2023 18:40

sanderblue force-pushed the main branch 2 times, most recently from 23a73db to 4f7b20e Compare May 7, 2024 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Dashboard json): Add parameter for query accounts #2454

feat(Dashboard json): Add parameter for query accounts #2454

alvaroaleman commented Sep 3, 2023 •

edited

Loading

CLAassistant commented Sep 3, 2023 •

edited

Loading

codecov-commenter commented Sep 21, 2023 •

edited

Loading

pranav-new-relic commented Sep 21, 2023 •

edited

Loading

This comment was marked as outdated.

alvaroaleman commented Sep 22, 2023

pranav-new-relic commented Sep 22, 2023

alvaroaleman commented Sep 22, 2023 •

edited

Loading

alvaroaleman commented Sep 22, 2023 •

edited

Loading

pranav-new-relic commented Sep 25, 2023

This comment was marked as outdated.

This comment was marked as outdated.

mbazhlekova Sep 28, 2023

alvaroaleman Sep 28, 2023

mbazhlekova commented Sep 28, 2023 •

edited

Loading

alvaroaleman commented Sep 28, 2023

mbazhlekova commented Sep 28, 2023

alvaroaleman commented Sep 28, 2023

mbazhlekova commented Sep 29, 2023

pranav-new-relic commented Sep 29, 2023 •

edited

Loading

alvaroaleman commented Sep 29, 2023

mbazhlekova commented Oct 2, 2023

pranav-new-relic commented Oct 10, 2023 •

edited

Loading

alvaroaleman commented Oct 11, 2023

	return fmt.Errorf("dashboard '%s' did not include updated accountID $d", string(serialized), dataAccountID)
	return fmt.Errorf("dashboard '%s' did not include updated accountID %s", string(serialized), dataAccountID)

feat(Dashboard json): Add parameter for query accounts #2454

Are you sure you want to change the base?

feat(Dashboard json): Add parameter for query accounts #2454

Conversation

alvaroaleman commented Sep 3, 2023 • edited Loading

Type of change

Checklist:

How to test this change?

CLAassistant commented Sep 3, 2023 • edited Loading

codecov-commenter commented Sep 21, 2023 • edited Loading

Codecov Report

pranav-new-relic commented Sep 21, 2023 • edited Loading

This comment was marked as outdated.

alvaroaleman commented Sep 22, 2023

pranav-new-relic commented Sep 22, 2023

alvaroaleman commented Sep 22, 2023 • edited Loading

alvaroaleman commented Sep 22, 2023 • edited Loading

pranav-new-relic commented Sep 25, 2023

This comment was marked as outdated.

This comment was marked as outdated.

mbazhlekova Sep 28, 2023

Choose a reason for hiding this comment

alvaroaleman Sep 28, 2023

Choose a reason for hiding this comment

mbazhlekova commented Sep 28, 2023 • edited Loading

alvaroaleman commented Sep 28, 2023

mbazhlekova commented Sep 28, 2023

alvaroaleman commented Sep 28, 2023

mbazhlekova commented Sep 29, 2023

pranav-new-relic commented Sep 29, 2023 • edited Loading

alvaroaleman commented Sep 29, 2023

mbazhlekova commented Oct 2, 2023

pranav-new-relic commented Oct 10, 2023 • edited Loading

1. accountId vs accountIds

2. The Validation Used In The Test Case

3. The Mock Account ID 123456789 Used In The Test

alvaroaleman commented Oct 11, 2023

alvaroaleman commented Sep 3, 2023 •

edited

Loading

CLAassistant commented Sep 3, 2023 •

edited

Loading

codecov-commenter commented Sep 21, 2023 •

edited

Loading

pranav-new-relic commented Sep 21, 2023 •

edited

Loading

alvaroaleman commented Sep 22, 2023 •

edited

Loading

alvaroaleman commented Sep 22, 2023 •

edited

Loading

mbazhlekova commented Sep 28, 2023 •

edited

Loading

pranav-new-relic commented Sep 29, 2023 •

edited

Loading

pranav-new-relic commented Oct 10, 2023 •

edited

Loading

1. `accountId` vs `accountIds`

3. The Mock Account ID `123456789` Used In The Test