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

feat(nrql_alert_condition): Add support for data account ID #2706

Conversation

founddrama
Copy link
Contributor

Description

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist:

  • My commit message follows conventional commits
  • My code is formatted to Go standards
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes. Go here for instructions on running tests locally.

How to test this change?

  • Add a nrql_alert_condition with a data_account_id -- the data_account_id may be the same account ID as the alert condition, or it may be an account ID that is in the same organization (the condition should create successfully)
  • Update nrql_alert_condition to have a data_account_id -- given a nrql_alert_condition created without a data_account_id, add one; given a nrql_alert_condition created with a data_account_id, change it (the condition should update successfully)
  • Add a nrql_alert_condition without a data_account_id -- it's an optional field, so you should be able to create a condition "the old fashioned way" (i.e., without specifying one -- it'll just inherit the account ID that is otherwise used to create the condition)

go.mod Outdated
@@ -7,7 +7,7 @@ require (
github.com/mitchellh/go-homedir v1.1.0
github.com/newrelic/go-agent/v3 v3.30.0
github.com/newrelic/go-insights v1.0.3
github.com/newrelic/newrelic-client-go/v2 v2.37.0
github.com/newrelic/newrelic-client-go/v2 v2.39.1
Copy link
Contributor Author

@founddrama founddrama Jul 2, 2024

Choose a reason for hiding this comment

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

Type: schema.TypeInt,
Optional: true,
Computed: true,
Description: "BETA PREVIEW: the `data_account_id` field is in limited release and only enabled for preview on a per-account basis. - The New Relic account ID to use as the basis for the NRQL alert condition's `query`; will default to `account_id` if unspecified.",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

📝 Calling out here that it's in "limited release"; after we GA the field, we'll update the Description.

@@ -3,6 +3,7 @@ package newrelic
import (
"context"
"fmt"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤷🏼‍♂️ gofmt did this ... I just agreed to it.

@@ -101,6 +101,7 @@ The following arguments are supported:
The `nrql` block supports the following arguments:

- `query` - (Required) The NRQL query to execute for the condition.
- `data_account_id` - (Optional) **BETA PREVIEW: the `data_account_id` field is in limited release and only enabled for preview on a per-account basis.** The account ID to use for the alert condition's query as specified in the the `query` field. If `data_account_id` is not specified, then the condition's query will be evaluated against the `account_id`. Note that the `account_id` must have read privileges for the `data_account_id` or else the condition will be invalid.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

📝 Calling out here that it's in "limited release"; after we GA the field, we'll update the Description.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.

Project coverage is 37.09%. Comparing base (92361de) to head (0eda853).
Report is 43 commits behind head on main.

Files Patch % Lines
...wrelic/structures_newrelic_nrql_alert_condition.go 62.50% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2706      +/-   ##
==========================================
+ Coverage   32.82%   37.09%   +4.27%     
==========================================
  Files          98       98              
  Lines       26884    21948    -4936     
==========================================
- Hits         8824     8142     -682     
+ Misses      17902    13645    -4257     
- Partials      158      161       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pranav-new-relic
Copy link
Member

performing some final checks before merge ....

@pranav-new-relic pranav-new-relic merged commit c9b4bc8 into newrelic:main Jul 3, 2024
11 checks passed
@pranav-new-relic
Copy link
Member

(performing final checks on main. Will push for a release once checked in the next hour)

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.

3 participants