-
Notifications
You must be signed in to change notification settings - Fork 880
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: support a custom base URL for the new relic provider #1053
Conversation
enables use of the provider by people accessing new relic via a proxy Signed-off-by: Fischer Jemison <[email protected]>
4b495ad
to
c9f9296
Compare
Codecov Report
@@ Coverage Diff @@
## master #1053 +/- ##
==========================================
+ Coverage 80.90% 81.01% +0.10%
==========================================
Files 103 103
Lines 9104 9165 +61
==========================================
+ Hits 7366 7425 +59
- Misses 1244 1245 +1
- Partials 494 495 +1
Continue to review full report at Codecov.
|
Based on discussion in slack: https://argoproj.slack.com/archives/CKTN88XFH/p1616688533036100 I don't think there's a corresponding issue but I can open one if that would be helpful |
@jemisonf if you could associate an issue with this, yes, that would be great, thanks. |
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.
This looks good to go. Do you mind also documenting this as well?
Taking some time to investigate if using |
metricproviders/newrelic/newrelic.go
Outdated
region := "us" | ||
if _, ok := secret.Data["region"]; ok { | ||
region = string(secret.Data["region"]) | ||
} | ||
newrelicOptions = append(newrelicOptions, newrelic.ConfigRegion(region)) | ||
|
||
// base URL for the new relic REST API | ||
if _, ok := secret.Data["base-url-rest"]; ok { | ||
newrelicOptions = append(newrelicOptions, newrelic.ConfigBaseURL(string(secret.Data["base-url-rest"]))) | ||
} | ||
|
||
// base URL for the nerdgraph (graphQL) API | ||
if _, ok := secret.Data["base-url-nerdgraph"]; ok { | ||
newrelicOptions = append(newrelicOptions, newrelic.ConfigNerdGraphBaseURL(string(secret.Data["base-url-nerdgraph"]))) |
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.
this is a change from the original approach, based on reading the docs. Looks like we need to define separate REST API and NerdGraph (aka GraphQL) API base URLs in addition to the region.
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.
there are some other base URL config keys available but these are the only two that should be relevant for this specific application
Signed-off-by: Fischer Jemison <[email protected]>
e79fd17
to
347816f
Compare
SonarCloud Quality Gate failed. 0 Bugs No Coverage information |
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.
LGTM!
Enables use of the provider by people accessing new relic via a proxy. Users should set either a
region
key or abase-url
key in their new relic secret, and if neither is provided the provider will continue to default to the US region.Checklist:
"fix(controller): Updates such and such. Fixes #1234"
.