Skip to content

Conversation

@Hweinstock
Copy link
Contributor

@Hweinstock Hweinstock commented Mar 11, 2025

Problem

app runner still uses sdkv2.

Solution

  • Refactor getServiceSummaries in AppRunnerNode to use SDK native pagination. This includes updates to the tests for AppRunnerNode.
  • As is usual with these migrations, build types to account for optional props.
  • Fix bug where wizard displayed node12 as label for node16 runtime.

Verification

  • Followed the AppRunner wizard to create an AppRunner deployment through the toolkit using public ECR (https://gallery.ecr.aws/aws-containers/hello-app-runner). Followed similar steps through the console to verify it worked the same way.
  • Pause and resume service via explorer.
  • Delete service via explorer.
  • Click in links via explorer.
  • used the cwl integration to search log streams associated with services.
  • used the live tail feature on the service (very cool!)

  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Hweinstock Hweinstock changed the title deps(apprunner): migrate to sdkv3. (WIP) deps(apprunner): migrate to sdkv3. Mar 12, 2025
@Hweinstock Hweinstock marked this pull request as ready for review March 12, 2025 18:44
@Hweinstock Hweinstock requested a review from a team as a code owner March 12, 2025 18:44
@Hweinstock Hweinstock changed the title deps(apprunner): migrate to sdkv3. refactor(apprunner): migrate to sdkv3. Mar 12, 2025
Comment on lines 201 to 212
const subform = new WizardForm<AppRunner.CodeRepository>()
function createCodeRepositorySubForm(git: GitExtension): WizardForm<AppRunnerCodeRepository> {
Copy link
Contributor

Choose a reason for hiding this comment

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

for future reference: the AppRunner.foo was intentional, to avoid needing to import everything explicitly. is that not possible with the new sdk? it's useful for discovery, e.g. one can do AppRunner.<tab> to complete other members, and also avoids noise in the imports list.

Copy link
Contributor Author

@Hweinstock Hweinstock Mar 12, 2025

Choose a reason for hiding this comment

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

I don't believe SDKv3 exports a namespace containing its corresponding types. It seems this is done intentionally to reduce bundle-size. (source: https://aws.amazon.com/blogs/developer/modular-packages-in-aws-sdk-for-javascript/). We could alternatively import the whole package and take the trade off of losing potential tree shaking benefits in favor of readability with something like:

import * as AppRunner from '@aws-sdk/client-apprunner'

Copy link
Contributor

Choose a reason for hiding this comment

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

We could alternatively import the whole package and take the trade off of losing potential tree shaking benefits in favor of readability with something like:

That is the usual pattern, yes. I didn't know it could interfere with tree-shaking... do you have a reference I can read about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call out.

I was confused because in the article I linked they say

The v3 also lets you import a modular bare-bones client with specific commands that help further reduce application bundle size!

However, this only applies to the CommonJS require syntax since it resolves dynamically. My understanding is that since ES Module is resolved statically, webpack can drop the unused pieces as dependencies in the bundle regardless of which import style we use.
(based on discussions here: https://stackoverflow.com/questions/46677752/the-difference-between-requirex-and-import-x and https://webpack.js.org/guides/tree-shaking/)

SourceCodeVersion: RequiredProps<SourceCodeVersion, 'Type' | 'Value'>
CodeConfiguration: AppRunnerCodeConfiguration
}
export interface AppRunnerSourceConfiguration extends SourceConfiguration {
Copy link
Contributor

@justinmk3 justinmk3 Mar 12, 2025

Choose a reason for hiding this comment

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

I would not bother with the AppRunnerSource prefix. And there would be no conflict if we did it like this:

Suggested change
export interface AppRunnerSourceConfiguration extends SourceConfiguration {
export interface SourceConfiguration extends AppRunner.SourceConfiguration {

It's probably important to fix this, or at least for any remaining migrations, because people copy whatever is already being done in clients. I don't think we want to add prefixes to every type name.

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 agree its not a great solution. One alternative would be to shadow the names of the underlying types, and import them as different names. Ex.

import { SourceConfiguration as SourceConfigurationSDK } from '@aws-sdk/client-apprunner'
...
export interface SourceConfiguration extends SourceConfigurationSDK {

My main reason for not doing this is that it could create confusion in cases where the custom type doesn't completely mirror the sdk type. As an example, in EC2 we add a LastSeenStatus field to the wrapping type that doesn't exist on the sdk type.

Copy link
Contributor

@justinmk3 justinmk3 Mar 13, 2025

Choose a reason for hiding this comment

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

it could create confusion in cases where the custom type doesn't completely mirror the sdk type.

The types are coming from this module, thus developers must understand they aren't the same. They have the same names, but developers are expected to understand that any module can use conflicting names anywhere. If one imports Foo from module A, that's unrelated to Foo from module B, even though they have the same name.

And in this codebase, the sdk wrappers/clients are intentionally imperfect projections of the sdk types. That is a fact of life for any application that wraps things.

LMK if I misunderstood

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, and seems in-line with how we construct the clients. I'll adjust the names of the types in this PR and look into a followup to refactor the existing work done.

@justinmk3
Copy link
Contributor

Could merge today since it doesn't affect Q.

@Hweinstock
Copy link
Contributor Author

/runIntegrationTests

@Hweinstock Hweinstock merged commit db491de into aws:master Mar 21, 2025
29 of 32 checks passed
@Hweinstock Hweinstock deleted the sdkv3/apprunner branch March 21, 2025 19:05
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.

2 participants