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 Legacy Enums #3235

Merged
merged 2 commits into from
Mar 26, 2024
Merged

Remove Legacy Enums #3235

merged 2 commits into from
Mar 26, 2024

Conversation

dscpinheiro
Copy link
Contributor

@dscpinheiro dscpinheiro commented Mar 26, 2024

Description

Removes the RequestRetryMode.Legacy and DefaultConfigurationMode.Legacy enum values; in V4, the .NET SDK will default to Standard.

There are 2 commits in this PR:

  • [Must be reviewed] Manual changes to update the enums: 0d23038
  • Generated changes to the DefaultConfiguration class for all services: 4f56a07

Testing

  • Built the AWSSDK.CoreAndCustomUnitTests.NetFramework.sln and AWSSDK.NetFramework.sln solutions
  • Ran the unit tests for the custom solution; there are a few failures but those happen in the v4 branch as well:
    • RetryForHttpStatus200WithErrorResponse
    • LongerTokenCaseAsync
    • MinimalBearerAuthCaseAsync
    • SingerShouldOverrideExistingHeaderAsync

License

  • I confirm that this pull request can be released under the Apache 2 license

@dscpinheiro dscpinheiro added the v4 label Mar 26, 2024
@dscpinheiro dscpinheiro requested a review from normj March 26, 2024 00:20
@@ -19,15 +19,16 @@ public class EndpointResolverTests
private const string AwsStsRegionalEndpointsEnvironmentVariable = "AWS_STS_REGIONAL_ENDPOINTS";
private const string DefaultStsEndpoint = @"https://sts.amazonaws.com/";

[TestMethod][TestCategory("UnitTest")]
[TestMethod]
[TestCategory("UnitTest")]
[TestCategory("Runtime")]
public void TestSuccessfulCall()
{
var endpointResolver = new EndpointResolver();
var executionContext = CreateExecutionContext(SetupConfig());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The config returned by the setup method now uses the Standard mode, so the determined endpoints have changed.

@@ -16,14 +16,14 @@
using Amazon.Runtime;
using Amazon;
using Moq;
using Xunit;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class was using xUnit, but the project is configured for MSTest so I think the tests weren't running... I updated it to match the other custom tests.

@@ -720,33 +711,6 @@ protected ClientConfig(IDefaultConfigurationProvider defaultConfigurationProvide
Initialize();
}

public ClientConfig() : this(new LegacyOnlyDefaultConfigurationProvider())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This constructor was not used by the service clients; I did have to modify one test to pass in a mocked provider.

@dscpinheiro dscpinheiro marked this pull request as ready for review March 26, 2024 00:38
@muhammad-othman muhammad-othman self-requested a review March 26, 2024 17:27
EnrichLegacyMode(parsedModel);


var validModes = parsedModel.Modes.Where(m => m.Name.ToPascalCase() != "Legacy").ToList();
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we should add this part, maybe we will need to add a new mode named Legacy in the future for any reason.

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 that's unlikely, Legacy is already one of the modes (https://docs.aws.amazon.com/sdkref/latest/guide/feature-smart-config-defaults.html), if a new one is added in the future we'd have to pick a new name.

Either way, it'd require us to update generator/ServiceClientGeneratorLib/DefaultConfiguration/DefaultConfigurationModel.cs as well.

@dscpinheiro dscpinheiro merged commit 03cf91a into v4-development Mar 26, 2024
3 checks passed
@dscpinheiro dscpinheiro deleted the dspin/DOTNET-7429 branch March 26, 2024 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants