Skip to content
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

Azure identity second try #394

Merged
merged 30 commits into from
Nov 26, 2024
Merged

Azure identity second try #394

merged 30 commits into from
Nov 26, 2024

Conversation

AsafMah
Copy link
Contributor

@AsafMah AsafMah commented Nov 4, 2024

No description provided.

AsafMah added 8 commits May 22, 2024 17:07
# Conflicts:
#	.github/workflows/release.yml
#	data/src/main/java/com/microsoft/azure/kusto/data/auth/ApplicationKeyTokenProvider.java
#	data/src/main/java/com/microsoft/azure/kusto/data/auth/DeviceAuthTokenProvider.java
#	data/src/main/java/com/microsoft/azure/kusto/data/auth/UserPromptTokenProvider.java
#	data/src/main/java/com/microsoft/azure/kusto/data/instrumentation/MonitoredActivity.java
#	data/src/test/java/com/microsoft/azure/kusto/data/auth/AadAuthenticationHelperTest.java
#	ingest/src/test/java/com/microsoft/azure/kusto/ingest/E2ETest.java
Copy link

github-actions bot commented Nov 4, 2024

Test Results

327 tests  +1   319 ✅ +1   3m 12s ⏱️ -34s
 27 suites ±0     8 💤 ±0 
 27 files   ±0     0 ❌ ±0 

Results for commit 9faae10. ± Comparison against base commit d1db64a.

♻️ This comment has been updated with latest results.

} catch (Exception e) {
throw new DataClientException(clusterUrl, ExceptionsUtils.getMessageEx(e), e);
}
protected Mono<String> acquireAccessTokenImpl() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see no reason to to use it like that

@@ -141,7 +141,8 @@ public static void setUp() {
@AfterAll
public static void tearDown() {
try {
queryClient.executeToJsonResult(DB_NAME, String.format(".drop table %s ifexists skip-seal", tableName));
new ExponentialRetry<DataServiceException, DataClientException>(2)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did it fail for you?>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's an holdover from when the tests failed, I'll undo it

@georgebanasios
Copy link
Contributor

georgebanasios commented Nov 21, 2024

Hello @AsafMah , @ohadbitt !

Apologies for interrupting your PR. I just wanted to ask you about the async operations, as I noticed Asaf made some steps in that direction in this PR. If you'd prefer to continue this discussion in another section, please let me know!

Are async operations still on the roadmap for the SDK? I've implemented changes to enable the Client interface to fully support async operations (using Project Reactor's Mono, just like the Azure core HTTP client returns). This includes making all Client methods asynchronous (on authentication as well, though I'm currently waiting for updates from this PR before finalizing that part).
At the moment, I've kept the sync methods as they were (which will eventually just wrap the async equivalents and block) and added the corresponding asynchronous ones. The E2E test seems to pass (utilizing the async methods), so I believe this is a solid first step.

I understand that this type of change requires extensive testing, but I would be willing to continue working on it and submit a draft PR for your review to showcase the changes. Let me know if this aligns with your plans, or if you’d prefer to approach this differently.

Thank you!

@AsafMah
Copy link
Contributor Author

AsafMah commented Nov 24, 2024

@georgebanasios

We do want to move to async, any contribution would be welcome.

@georgebanasios
Copy link
Contributor

@AsafMah

Great! After your PR, I'll finish up with authentication and continue with ingest. I'll keep you guys updated!

@AsafMah AsafMah merged commit 3714784 into master Nov 26, 2024
8 checks passed
@AsafMah AsafMah deleted the azure-identity02 branch November 26, 2024 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants