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

Remove duplicate scopes value in reference.conf #314

Conversation

mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Jan 14, 2024

While working on #313 I realized that we have duplicate scope configuration settings, i.e. one here and another here.

This PR removes the scopes config here and instead makes the ServiceAccountCredentials use the scopes value from here which is currently unused.

We also change the collection from a Seq/List to a Set since we want duplicates to be removed and ordering does not matter (also to make it consistent with #313)

@mdedetrich mdedetrich added this to the 1.1.0 milestone Jan 14, 2024
@mdedetrich mdedetrich force-pushed the remove-duplicate-scopes-value-reference-conf branch from d5d5849 to 9c691b2 Compare January 14, 2024 08:25
@mdedetrich mdedetrich added the release note should be mentioned in release notes label Jan 14, 2024
@mdedetrich mdedetrich force-pushed the remove-duplicate-scopes-value-reference-conf branch from 9c691b2 to 2f82489 Compare January 14, 2024 08:31
@mdedetrich
Copy link
Contributor Author

mdedetrich commented Jan 14, 2024

Pinging @armanbilge since you originally wrote this, not sure if you wanted to throw in a quick 2c

@pjfanning
Copy link
Contributor

you'll need something like https://github.com/apache/incubator-pekko-connectors/pull/311/files#diff-5634c415cd8c8504fdb973a3ed092300b43c4b8fc1e184f7249eb29a55511f91R16 to get the mima check to work - v1.0.2-RC1 is current latest tag and this is only in staging repo - this PR looks like it could have a big mima footprint

@mdedetrich
Copy link
Contributor Author

This PR doesn't have any MiMa impact, ill just rebase when 1.0.2 is released

@@ -37,7 +37,7 @@ private[auth] object GoogleOAuth2 {

private val oAuthTokenUrl = "https://oauth2.googleapis.com/token"

def getAccessToken(clientEmail: String, privateKey: String, scopes: Seq[String])(
def getAccessToken(clientEmail: String, privateKey: String, scopes: Set[String])(
Copy link
Member

Choose a reason for hiding this comment

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

I think a mina filter will be needed. or add an overloaded method which use seq.toSet then?

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'll wait till 1.0.2 is out since that will fix the existing MiMa issue and then I will see.if this is still problematic

Copy link
Member

@jxnu-liguobin jxnu-liguobin left a comment

Choose a reason for hiding this comment

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

lgtm

@mdedetrich mdedetrich force-pushed the remove-duplicate-scopes-value-reference-conf branch from 2f82489 to fd460e2 Compare January 25, 2024 22:42
Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

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

Lgtm

@mdedetrich
Copy link
Contributor Author

Merging this since 1.0.x branch exists

@mdedetrich mdedetrich merged commit 26fa174 into apache:main Jan 27, 2024
50 checks passed
@mdedetrich mdedetrich deleted the remove-duplicate-scopes-value-reference-conf branch January 27, 2024 21:59
@JD557 JD557 mentioned this pull request Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release note should be mentioned in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants