-
Notifications
You must be signed in to change notification settings - Fork 388
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
OAK-10615 - Azure Service Principal Support in oak-run segment-copy, compact, console #1280
Conversation
First un-tested implementation
…of service principal
Bumped azurite version to 3.29.0 Increased oak.segment.azure package version after refactorings
5c50b86
to
6cdf173
Compare
Increased again oak-run size to include apache commons lang3 needed for running azure commands
|
||
assertEquals("myaccount", credentials.getAccountName()); | ||
|
||
assertTrue(checkLogContainsMessage(AZURE_SECRET_KEY_WARNING, logAppender.list.stream().map(ILoggingEvent::getFormattedMessage).collect(Collectors.toList()))); |
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 am not in favor of this type of check. Instead, I would prefer to examine the concrete class returned (subtype of StorageCredentials) or use another method for verification.
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.
Ack. I tried now to tweak it via concrete class returned, as you mentioned, but that would be irrelevant IMO, since we already know that we expect StorageCredentialsAccountAndKey
. To me, checking the logs for the warning is still valuable, as this log could witness later deprecated authentication method used.
We use the same mechanism here: DefaultSegmentWriterTest#testHugeMapRecordErrorSize.
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.
Thanks for the additional info.
In another task, we can perhaps evaluate other options to perform verification of such nature.
First un-tested implementation