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

fix: profile credentials provider needing opt in to internal annotation #1215

Merged
merged 4 commits into from
Feb 16, 2024

Conversation

0marperez
Copy link
Contributor

@0marperez 0marperez commented Feb 12, 2024

Issue #

closes #1208

Description of changes

One of the constructor parameters was marked as internal SDK API. Which was causing the profile credentials provider to be marked as internal API.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK

@0marperez 0marperez marked this pull request as ready for review February 12, 2024 19:42
@0marperez 0marperez requested a review from a team as a code owner February 12, 2024 19:42
Comment on lines 76 to 79
/**
* Specifies the active profile and configured (may not actually exist) locations of configuration files.
*/
@InternalSdkApi
public data class AwsConfigurationSource(val profile: String, val configPath: String, val credentialsPath: String)
Copy link
Contributor

Choose a reason for hiding this comment

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

Correctness: I believe we still want this type to be @InternalSdkApi. I think instead we should add to ProfileCredentialsProvider a new constructor overload that doesn't have an AwsConfigurationSource parameter. Then we can mark the current constructor as @InternalSdkApi.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about if the user wants to configure the AwsConfigurationSource?

Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK

Copy link

sonarcloud bot commented Feb 14, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK

Copy link
Collaborator

@aajtodd aajtodd left a comment

Choose a reason for hiding this comment

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

Looks good just one question on whether we verified the behavior change.

public val profileName: String? = null,
public val region: String? = null,
public val platformProvider: PlatformProvider = PlatformProvider.System,
public val httpClient: HttpClientEngine? = null,
public val configurationSource: AwsConfigurationSource? = null,
) : CloseableCredentialsProvider {

public constructor(
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: Have you tried this out by publishing it to maven local and trying to use it in a project? I tested the issue originally to verify and could see intellij correctly flagged it, we should verify this actually fixes the issue for customers by comparing before and after if you haven't already.

Copy link
Contributor Author

@0marperez 0marperez Feb 15, 2024

Choose a reason for hiding this comment

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

Yes, I published it to maven local and gave it a try.

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 could see the warning go away and there was no longer a need to import aws.smithy.kotlin:http-client

@0marperez 0marperez dismissed ianbotsf’s stale review February 16, 2024 18:23

Changes requested were made and received two approvals from rest of team.

@0marperez 0marperez merged commit 13d60e3 into main Feb 16, 2024
16 of 17 checks passed
@0marperez 0marperez deleted the profile-cred-prov-fix branch February 16, 2024 18:26
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.

ProfileCredentialsProvider isn't available without internal modules
4 participants