Skip to content

Conversation

@lachriz-aws
Copy link
Contributor

The default runtime was set to DOTNET_6 while the docs said PROVIDED_AL2. This PR streamlines this, and changes the default runtime to be DOTNET_8.

Copy link
Contributor

@vlesierse vlesierse left a comment

Choose a reason for hiding this comment

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

With .NET 8 as default and the out of support of .NET 6 in November, this fix makes sense. It is a breaking change though, so we should mark it as features or include this in the other PR #259 with a minor version bump.

@lachriz-aws
Copy link
Contributor Author

lachriz-aws commented Sep 5, 2024

With .NET 8 as default and the out of support of .NET 6 in November, this fix makes sense. It is a breaking change though, so we should mark it as features or include this in the other PR #259 with a minor version bump.

Now that #259 is marked as a fix, I think it would make sense to follow your proposal of changing this PR (#260) to be a feature - and hence have them tracked as two different changes. I will change that now.

In terms of versioning - the current version is 0.0.4. Reading you suggestion to bump the minor version -- from a semantic versioning point of view, that would imply we do a 0.1.0 release, wouldn't it? Alternatively, we could also consider discussing if we instead want to do a first stable release (1.0.0) and have this PR included as part of that release?`

Many options to choose from - let me know what you think.

@lachriz-aws lachriz-aws changed the title fix: Updated (and streamlined) the default runtime to be DOTNET_8 feat: Update (and streamline) the default runtime to be DOTNET_8 Sep 5, 2024
@vlesierse
Copy link
Contributor

@lachriz-aws we can leave both PR separate. For versioning, currently this library is marked as experimental by the CDK team. We can take this to them on what is needed to bring it to a stable 1.0 release. Meanwhile following semantic versioning 0.1.0 would be a good next version for this feature.

@lachriz-aws
Copy link
Contributor Author

lachriz-aws commented Sep 17, 2024

Thanks for the above feedback @vlesierse. The PR now contains the following changes:

  • The default runtime is now .NET 8.
  • The lambda-handler-aot integ test project has been upgraded to .NET 8 (from .NET 7 which is end of life).
  • The lambda-handler-docker integ test project has been upgraded to .NET 8 (from .NET 6).
  • The lambda-handler integ test project has been upgraded to .NET 8 (from .NET 6).

Also, a new integ test project lambda-handler-dotnet6 has been added to test the use of a non-default .NET managed runtime (as per your feedback).

Finally, I should mention that the lambda-handler-aot integ test was failing when deployed from Windows x86_64 due to the following runtime identifier condition which caused the libicu dependency to not be bundled with the Lambda fn:

<ItemGroup Condition="'$(RuntimeIdentifier)' == 'linux-arm64'">
  <RuntimeHostConfigurationOption Include="System.Globalization.AppLocalIcu" Value="68.2.0.9" />
  <PackageReference Include="Microsoft.ICU.ICU4C.Runtime" Version="68.2.0.9" />
</ItemGroup>

Since globalization is not really core to the tests, I have simply replaced that inclusion with the below:

<!-- Disable globalization support to avoid having to take a dependency on libicu -->
<InvariantGlobalization>true</InvariantGlobalization>

Copy link
Contributor

@vlesierse vlesierse left a comment

Choose a reason for hiding this comment

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

LGTM

@aws-cdk-automation aws-cdk-automation added this pull request to the merge queue Sep 17, 2024
Merged via the queue into cdklabs:main with commit c0bc7c9 Sep 17, 2024
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.

3 participants