-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[MetricsAdvisor] Added tests for Data Feed CRUD operations (1/4) #17721
Conversation
|
||
var createdDataSource = createdDataFeed.DataSource as AzureApplicationInsightsDataFeedSource; | ||
|
||
var expectedKey = TestEnvironment.Mode == RecordedTestMode.Playback ? "Sanitized" : "key"; |
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.
Unfortunately we can't check the authentication strings in Data Feed tests, since they're always sanitized during recording. This issue is only present in Playback, though.
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.
So to make sure I understand, the service doesn't check that it can connect to the specified DataFeeed, right? which is why u can pass whatever?
Can anyone access the ApiKey
property? or the client needs to be created with a specific role or something for someone to get that value
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.
And the idea is not to suppress this sanitization in case someone uses a real key and then it gets logged?
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.
So to make sure I understand, the service doesn't check that it can connect to the specified DataFeeed, right? which is why u can pass whatever?
Correct.
Can anyone access the ApiKey property? or the client needs to be created with a specific role or something for someone to get that value
According to the service, it's only returned if you have the admin role for that Data Feed (which is always true for the person who created it).
And the idea is not to suppress this sanitization in case someone uses a real key and then it gets logged?
Exactly.
...dvisor/Azure.AI.MetricsAdvisor/tests/MetricsAdvisorAdministrationClient/DataFeedLiveTests.cs
Show resolved
Hide resolved
...dvisor/Azure.AI.MetricsAdvisor/tests/MetricsAdvisorAdministrationClient/DataFeedLiveTests.cs
Show resolved
Hide resolved
Assert.That(dataFeed.Name, Is.EqualTo(expectedName)); | ||
Assert.That(dataFeed.Description, Is.Not.Null.And.Empty); | ||
Assert.That(dataFeed.Status, Is.EqualTo(DataFeedStatus.Active)); | ||
Assert.That(dataFeed.AccessMode, Is.EqualTo(DataFeedAccessMode.Private)); |
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.
is this the default of the service? Should it actually be nullable on our side?
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.
We use the same model for the UpdateDataFeed
operation, and an AccessMode
doesn't need to be specified. So we're making it nullable.
...dvisor/Azure.AI.MetricsAdvisor/tests/MetricsAdvisorAdministrationClient/DataFeedLiveTests.cs
Show resolved
Hide resolved
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, just a bunch of unrelated behavior service questions
…Live, Create, Get) (Azure#17721)
Part of #15913.
This PR is part of a huge set of changes that fixes the issue mentioned above. These changes were separated into 4 chunks to reduce the review burden. This PR consists of: