Skip to content

Add support for reading images from AWS ECR #16

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

Merged
merged 12 commits into from
Jun 9, 2025
Merged

Conversation

ehdr
Copy link
Contributor

@ehdr ehdr commented May 27, 2025

CLI-433

Adds support similar to modal.Image.from_aws_ecr(), related support for Secrets.

@ehdr ehdr requested a review from freider May 27, 2025 11:28
@ehdr ehdr force-pushed the ehdr/add-image-from-aws-ecr branch from 71d8457 to a6c05a4 Compare May 27, 2025 11:31

async imageFromAwsEcr(tag: string, secret: Secret): Promise<Image> {
if (!secret.secretId) {
throw new Error("secret.secretId must not be null");
Copy link

Choose a reason for hiding this comment

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

This error could be a bit more helpful, like secret must be a reference to an existing Secret, e.g. await Secret.from_name("my_secret")or similar - I wouldn't expect users to know how to get an object with asecretId` here otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 ✅

Copy link
Member

@luiscape luiscape left a comment

Choose a reason for hiding this comment

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

Hi @ehdr, can you make the following changes:

  1. implement the same functionality in Go
  2. add tests to both secret and, ideally, imageFromAwsEcr
  3. add examples

Copy link
Member

@ekzhang ekzhang left a comment

Choose a reason for hiding this comment

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

Lgtm just need to follow some language conventions and add tests! I think you'd likely have noticed the camelCase naming thing after writing the examples / tests since they would be inconsistent with the code around it.

Also what Luis mentioned above, we're trying to keep the code as similar as possible between Go and TS and avoid it drifting.

Comment on lines 90 to 117
if (!secret.secretId) {
throw new Error(
"secret must be a reference to an existing Secret, e.g. `await Secret.from_name('my_secret')`",
);
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this validation is needed or helpful since the Secret object, once constructed properly, must have a secret ID already.

Copy link
Contributor Author

@ehdr ehdr May 27, 2025

Choose a reason for hiding this comment

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

I added it because it failed when I left out the await on Secret.from_name in await app.imageFromAwsEcr("alpine:3.21", await Secret.from_name("aws")). Do you think it makes sense, and/or is there a better way to guard for this?

Copy link
Member

Choose a reason for hiding this comment

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

Ah got it. Yeah that would be caught by TypeScript! A more direct check might be

if (!(secret instanceof Secret)) {
  throw new TypeError("...");
}

@ehdr
Copy link
Contributor Author

ehdr commented May 27, 2025

Thanks @luiscape @ekzhang , great comments (and sorry for the Python casing... 🤦 ). I'll attend to these tomorrow.

@ehdr ehdr force-pushed the ehdr/add-image-from-aws-ecr branch from a0ba483 to 4bb7820 Compare May 28, 2025 12:37
@ehdr
Copy link
Contributor Author

ehdr commented May 28, 2025

I fixed most of the comments now. Will have to pause this for a bit for customer related work, but will add example and update Go client later before merging!

Copy link
Member

@ekzhang ekzhang left a comment

Choose a reason for hiding this comment

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

Thank you for updating!

@ehdr ehdr force-pushed the ehdr/add-image-from-aws-ecr branch from 4bb7820 to c598abe Compare June 4, 2025 13:09
@ehdr ehdr changed the title Add App.imageFromAwsEcr for reading images from AWS ECR Add support for reading images from AWS ECR Jun 4, 2025
Copy link
Member

@luiscape luiscape left a comment

Choose a reason for hiding this comment

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

Go implementation lgtm.

RequiredKeys []string
}

func SecretFromName(ctx context.Context, name string, options SecretFromNameOptions) (*Secret, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Add documentation

@ehdr ehdr force-pushed the ehdr/add-image-from-aws-ecr branch from c598abe to c908344 Compare June 8, 2025 20:30
@ehdr
Copy link
Contributor Author

ehdr commented Jun 8, 2025

I finally (after some other distractions) got around to finishing this, so I think all outstanding comments are fixed now! I also adapted it to #23 (e.g. the Go SDK accepts nil as default value for options etc.).

@ehdr ehdr force-pushed the ehdr/add-image-from-aws-ecr branch 2 times, most recently from cac5072 to a4751e7 Compare June 8, 2025 20:54
Copy link
Member

@ekzhang ekzhang left a comment

Choose a reason for hiding this comment

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

LGTM!

@ehdr ehdr force-pushed the ehdr/add-image-from-aws-ecr branch from a4751e7 to 063e245 Compare June 9, 2025 17:39
@ehdr ehdr force-pushed the ehdr/add-image-from-aws-ecr branch from 5310419 to 8798e93 Compare June 9, 2025 18:39
@ehdr ehdr force-pushed the ehdr/add-image-from-aws-ecr branch from 8912234 to 49cc720 Compare June 9, 2025 21:21
@ehdr ehdr force-pushed the ehdr/add-image-from-aws-ecr branch from 49cc720 to 5302232 Compare June 9, 2025 21:22
Copy link
Member

@ekzhang ekzhang left a comment

Choose a reason for hiding this comment

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

Thank you!

Comment on lines 20 to 22
"*.config.ts",
"vitest.config.ts",
"tsup.config.ts"
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, good catch, fixed!

@ehdr ehdr merged commit 473a9e0 into main Jun 9, 2025
5 of 6 checks passed
@ehdr ehdr deleted the ehdr/add-image-from-aws-ecr branch June 9, 2025 21:45
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.

4 participants