-
Notifications
You must be signed in to change notification settings - Fork 75
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
Feature: AppDynamics data link #558
Feature: AppDynamics data link #558
Conversation
All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #558 +/- ##
=======================================
Coverage 94.87% 94.87%
=======================================
Files 35 35
Lines 2107 2107
=======================================
Hits 1999 1999
Misses 89 89
Partials 19 19 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a comment from me.
appdURLs := val.(*schema.Set).List() | ||
|
||
appdUrlPatternRegex := "^https?:\\/\\/[a-zA-Z0-9-]+\\.saas\\.appdynamics\\.com\\/.*application=\\d+.*component=\\d+.*" | ||
re := regexp.MustCompile(appdUrlPatternRegex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using MustCompile
or Must*
methods in general since on error they will panic causing state to be incorrectly set if this was to be in the middle in an apply.
Be sure to use Compile
instead and return the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind expanding on what the regular expression is testing?
Reading it it looks like you can have any wildcard for *.saas.appdynamics.com
and expecting to have the query values for application
, and component
?
Did I miss anything here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that is correct. The other condition would be that the query values for application
and component
need to be positive integers.
Feature
target_appd_url
to data link resourcesignalfx-go
versionUsage
target_appd_url
field on a data link resource.Implementation
target_external_url
,target_splunk
, etc.).target_appd_url
.saas.appdynamics.com
, query valuescomponent
andapplication
. The query values should be positive integersTesting
development_overrides
flag.target_appd_url
url that should create and destroy a data link without errorstarget_appd_url
with a missing query parametercomponent
. This should throw an error in the URL validation logic.