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.

Sign up for GitHub

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

Jump to bottom

Add tenantId to azure oauth2 native fs auth #20764

Merged
merged 1 commit into from
Feb 21, 2024
Merged

Add tenantId to azure oauth2 native fs auth #20764

merged 1 commit into from
Feb 21, 2024

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Feb 20, 2024 •
edited
Loading

@wendigo wendigo requested review from dain and findepi February 20, 2024 10:56
@cla-bot cla-bot bot added the cla-signed label Feb 20, 2024
@wendigo
Copy link
Contributor Author

wendigo commented Feb 20, 2024

It seems that this is required: Azure/azure-sdk-for-java#36155

@wendigo
Copy link
Contributor Author

wendigo commented Feb 20, 2024 

Learn more:
  https://github.com/google/guice/wiki/ERROR_INJECTING_CONSTRUCTOR
Caused by: IllegalArgumentException: Must provide non-null values for tenantId properties in ClientSecretCredentialBuilder
    at ValidationUtil.validate(ValidationUtil.java:50)
    at ClientSecretCredentialBuilder.build(ClientSecretCredentialBuilder.java:129)
    at AzureAuthOauth.<init>(AzureAuthOauth.java:39)
    at AzureAuthOauth.<init>(AzureAuthOauth.java:30)
    at AzureAuthOauth$$FastClassByGuice$$308b03b.GUICE$TRAMPOLINE(<generated>)
    at AzureAuthOauth$$FastClassByGuice$$308b03b.apply(<generated>)
    at DefaultConstructionProxyFactory$FastClassProxy.newInstance(DefaultConstructionProxyFactory.java:82)
    at ConstructorInjector.provision(ConstructorInjector.java:114)
    at ConstructorInjector.access$000(ConstructorInjector.java:33)
    at ConstructorInjector$1.call(ConstructorInjector.java:98)
    at ProvisionListenerStackCallback$Provision.provision(ProvisionListenerStackCallback.java:109)
    at LifeCycleModule.provision(LifeCycleModule.java:53)

@wendigo
Copy link
Contributor Author

wendigo commented Feb 20, 2024

We never tested this in the CI :)

@wendigo wendigo force-pushed the serafin/azure-oauth-tenantid branch from 74578c5 to b3bd14c Compare February 20, 2024 11:02
@findepi
Copy link
Member

findepi commented Feb 20, 2024

We never tested this in the CI :)

Let's fix that

@wendigo wendigo force-pushed the serafin/azure-oauth-tenantid branch from b3bd14c to b63c5db Compare February 20, 2024 11:21
@wendigo wendigo force-pushed the serafin/azure-oauth-tenantid branch from b63c5db to 2caf91d Compare February 20, 2024 13:08
@hashhar hashhar mentioned this pull request Feb 21, 2024
Closed
findinpath
findinpath reviewed Feb 21, 2024
View reviewed changes
lib/trino-filesystem-azure/src/main/java/io/trino/filesystem/azure/AzureAuthOAuthConfig.java
return tenantId;
}

@Config("azure.oauth.tenant-id")
Copy link
Contributor

@findinpath findinpath Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Please add @ConfigDescription here.
I see that the whole class is missing descriptions - pls consider adding them if you're aware of them. Otherwise, we can do them in a follow-up PR

Copy link
Member

Choose a reason for hiding this comment

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

let's do as a follow-up

@findepi findepi merged commit 3cd639f into master Feb 21, 2024
69 checks passed
@findepi findepi deleted the serafin/azure-oauth-tenantid branch February 21, 2024 08:25
@github-actions github-actions bot added this to the 440 milestone Feb 21, 2024
@colebow colebow mentioned this pull request Feb 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@findinpath findinpath findinpath left review comments

@dain dain Awaiting requested review from dain

@findepi findepi Awaiting requested review from findepi

Assignees
No one assigned
Labels
cla-signed docs
Milestone
440
Development

Successfully merging this pull request may close these issues.
3 participants
@wendigo @findepi @findinpath