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

feat: sam Commands Understand Local File Paths for AWS::Serverless::Function.ImageUri #6930

Merged
merged 12 commits into from
May 1, 2024

Conversation

chrisoverzero
Copy link
Contributor

@chrisoverzero chrisoverzero commented Apr 10, 2024

As a summary, sam has learned to load an an image from a local archive before proceeding with the build, package, and deploy commands.

When running sam build with an ImageUri pointing to a local file, sam will load that archive into an image, then write the ID of the image to the ImageUri property of the built template. ID works the same as a tag for the Docker API, so business continues as usual from here. The reason behind writing ID is that a loaded image could be associated with multiple tags, and selecting one arbtrarily leads to difficulties in the deploy command.

The package and deploy commands have three kinds of value for ImageUri to consider. First, a value of the form {repo}:{tag}. This functions as it always has. Second, an image ID (in the form sha256:{digest}) which is probably the output of sam build. In this case, the tag translation uses the name of the as its input. Otherwise, they'd all end up with names starting with "sha256". Last, a local file. In this case, it proceeds as it does in the build command: Load the archive into an image first, then pass the resource name into tag translation.

See: #6909

Which issue(s) does this change fix?

#6909

Why is this change necessary?

From the initial ticket:

W-Well, I want this because my build system is capable of creating Docker images as file archives without the docker command-line tool or the docker service available. I know of other development teams who produce file archives of Docker images for scanning purposes. Lacking either end of Docker simplifies those steps in the CI process, so we only need to associate the docker-in-docker service with the job in which we perform sam commands for the final deployment. Also, it's much easier in many ways to manage CI artifacts which are normal files.

How does it address the issue?

sam has learned to load an an image from a local archive before proceeding with the build, package, and deploy commands when such a local archive is the value of ImageUri for an AWS::Serverless::Function

What side effects does this change have?

The kinds of values understood for ImageUri has increased: A docker tag, a docker ID, and a local file archive. Though I think that a docker ID would always have worked, but unpleasantly. Either way, it wasn't documented and presumably not supported. Now it would be.

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • Add input/output type hints to new functions/methods
  • Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • Write/update integration tests
  • Write/update functional tests if needed
  • make pr passes
  • make update-reproducible-reqs if dependencies were changed
  • Write documentation

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

@chrisoverzero chrisoverzero requested a review from a team as a code owner April 10, 2024 15:48
@github-actions github-actions bot added area/local/start-api sam local start-api command area/build sam build command area/local/invoke sam local invoke command area/local/start-invoke pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Apr 10, 2024
@jysheng123
Copy link
Contributor

I think in general, the code is very good here and I don't see many major flaws, I do think however this does need some testing both unit/integration test wise. For testing purposes, we should ideally have a test for each potential path, like the happy path and each error handling that we have, like if the image path returns multiple files for example. The unit testing itself should just have some simple assert validation on errors and the function being called.

@chrisoverzero
Copy link
Contributor Author

I do think however this does need some testing both unit/integration test wise.

I agree and I'm happy to try. Much of the test code feels a bit beyond my current Python familiarity, so it could take me some time to figure it out.

@jysheng123
Copy link
Contributor

No worries take your time, I would be more than happy to take a look at your tests/ if you have any questions.

@jysheng123 jysheng123 added area/package sam package command area/docker area/deploy sam deploy command and removed area/local/start-api sam local start-api command area/local/invoke sam local invoke command area/local/start-invoke stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Apr 10, 2024
@mndeveci mndeveci removed the request for review from bentvelj April 10, 2024 18:43
As a summary, sam has learned to load an an image from a local archive
before proceeding with the `build`, `package`, and `deploy` commands.

When running `sam build` with an `ImageUri` pointing to a local file,
sam will load that archive into an image, then write the ID of the
image to the `ImageUri` property of the built template. ID works the
same as a tag for the Docker API, so business continues as usual from
here. The reason behind writing ID is that a loaded image could be
associated with multiple tags, and selecting one arbtrarily leads to
difficulties in the deploy command.

The package and deploy commands have three kinds of value for
`ImageUri` to consider. First, a value of the form
`{repo}:{tag}`. This functions as it always has. Second, an image
ID (in the form `sha256:{digest}`) which is probably the output of
`sam build`. In this case, the tag translation uses the name of the
as its input. Otherwise, they'd all end up with names starting with
"sha256". Last, a local file. In this case, it proceeds as it does in
the build command: Load the archive into an image first, then pass the
resource name into tag translation.

See: aws#6909
@chrisoverzero
Copy link
Contributor Author

@jysheng123 When I go to run integration tests, will that perform deployments, or would only end-to-end tests do that? I need to know if I should have a scratch account ready to go so I don't get the hey what is happening cloudtrail is going crazy with deployments to us-east-1 or something kind of Slack messages.

@jysheng123
Copy link
Contributor

Some of our integration tests will perform deployments, they should be deleted after the tests are done but if you want to be safe and not touch your actual account, I would recommend using another account

@chrisoverzero
Copy link
Contributor Author

Great, thanks so much. I will work on getting that set up.

Copy link
Contributor

@jysheng123 jysheng123 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 to me! Ill ask another developer to take a look at this, thanks for bringing up the PR

@sidhujus
Copy link
Contributor

Hi @chrisoverzero thanks for this PR and the thorough testing! The integration tests you added seem to be failing because the template is in the incorrect location. For these tests the test data lives under the package folder https://github.com/aws/aws-sam-cli/blob/develop/tests/integration/package/package_integ_base.py#L62 (the DeployIntegBase class inherits from the PackageIntegBase class. Could you move the required files into the correct location and then I can verify the new integration tests pass. Thanks!

@sidhujus
Copy link
Contributor

other than fixing the failing integration test and removing that one unused import the changes look good to me overall as well. Once thats fixed and we verify that the tests are all passing we can merge this in, thanks once again!

@sidhujus sidhujus enabled auto-merge May 1, 2024 16:38
@sidhujus sidhujus added this pull request to the merge queue May 1, 2024
Merged via the queue into aws:develop with commit e0bc44d May 1, 2024
55 of 57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build sam build command area/deploy sam deploy command area/docker area/package sam package command pr/external
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants