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
AzureMonitor: Deprecate using separate credentials for Azure Monitor Logs #34758
Conversation
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.
Haven't had a chance to test this out yet, but looks great so far (just with two super minor comments)
{showSameAsHelpMsg && ( | ||
<div className="grafana-info-box m-t-2"> | ||
<div className="alert-body"> | ||
<p>Re-enter your Azure Monitor Client Secret to use this setting.</p> | ||
</div> | ||
</div> | ||
)} |
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.
Include this in the condition as well - we don't want to show this if the switch isn't visible.
It might not in practice, but we want to be extra sure 😅
{showLogAnalyticsCreds && logAnalyticsCredentials && ( | ||
<> | ||
<Alert severity="info" title="Deprecated"> | ||
Using different credentials for Azure Monitor logs is deprecated and will be removed in a future |
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.
Capitalise "Logs".
onChange={onLogAnalyticsSameAsChange} | ||
{...tooltipAttribute} | ||
/> | ||
{showLogAnalyticsCreds && ( |
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.
The variable credentialsEnabled
above serves the same purpose - to hide the credentials section when not needed., You can change condition of that variaable instead.
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.
I would merge the two variables showLogAnalyticsCreds
and credentialsEnabled
into a one.
Something like this:
const [credentialsUsed] = useState(!!logAnalyticsCredentials);
const credentialsEnabled = credentialsUsed && primaryCredentials.authType === 'clientsecret';
Liked it! It's more clean now. BTW @joshhunt I migrated the tests to the @testing-library as suggested offline. That caused the snapshots to be quite different but it should be fine (TBH I'm not a fan of snapshots in any case). |
Yeah, I wouldn't fuss too much about the snapshot tests 🙄 |
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.
🚀
* Hide switch for using different credentials in AzureMonitor * Apply feedback. Use @testing-library (cherry picked from commit 48dc78f)
…34877) * Hide switch for using different credentials in AzureMonitor * Apply feedback. Use @testing-library (cherry picked from commit 48dc78f) Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>
What this PR does / why we need it:
Remove the switch that allow people to specify different credentials when creating an AzureMonitor data source:
To maintain backwards compatibility, if a data source already has different credentials, we still show the switch but with a deprecation notice:
Which issue(s) this PR fixes:
Fixes #34621
5th item at #34622
Special notes for your reviewer:
Let me know if you think of a better deprecation text.
cc/ @kostrse