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

[#5989] The credential type of ADLSTokenCredentialProvider is not equal to the credential type of ADLSTokenCredential #5990

Merged
merged 8 commits into from
Dec 28, 2024

Conversation

FANNG1
Copy link
Contributor

@FANNG1 FANNG1 commented Dec 25, 2024

What changes were proposed in this pull request?

ADLSTokenCredentialProvider use the credential type from ADLSTokenCredential

Why are the changes needed?

Fix: #5989

Does this PR introduce any user-facing change?

no

How was this patch tested?

add test

@FANNG1
Copy link
Contributor Author

FANNG1 commented Dec 25, 2024

@jerryshao @yuqi1129 @orenccl please help to review when you have time, thanks

@orenccl
Copy link
Collaborator

orenccl commented Dec 25, 2024

What happen when they are not equals?

@FANNG1
Copy link
Contributor Author

FANNG1 commented Dec 25, 2024

What happen when they are not equals?

no bad effect, but we'd better keep it consistent to make the system simple, The user create an credential provider with type A, we'd better produce the credential with type A too.

Copy link
Collaborator

@orenccl orenccl left a comment

Choose a reason for hiding this comment

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

Got it! LGTM

@tengqm
Copy link
Contributor

tengqm commented Dec 26, 2024

LGTM.

@FANNG1
Copy link
Contributor Author

FANNG1 commented Dec 26, 2024

updating the PR to replace adls-sas-token with adls-token for the credential type of ADLSTokenCredential to keep with other credential types like s3-token, oss-token, gcs-token, please help to review again

Copy link
Collaborator

@orenccl orenccl left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for fixing naming.

@tengqm
Copy link
Contributor

tengqm commented Dec 26, 2024

Nice cleanup.

@jerryshao
Copy link
Contributor

@FANNG1 can you please rebase the PR to fix the conflicts?

@FANNG1
Copy link
Contributor Author

FANNG1 commented Dec 27, 2024

@FANNG1 can you please rebase the PR to fix the conflicts?

updated

@jerryshao jerryshao merged commit 486c042 into apache:main Dec 28, 2024
26 checks passed
Abyss-lord pushed a commit to Abyss-lord/gravitino that referenced this pull request Dec 29, 2024
…ot equal to the credential type of ADLSTokenCredential (apache#5990)

### What changes were proposed in this pull request?

ADLSTokenCredentialProvider use the credential type from
ADLSTokenCredential

### Why are the changes needed?


Fix: apache#5989 

### Does this PR introduce _any_ user-facing change?
no

### How was this patch tested?
add test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants