Skip to content

Conversation

@joeyhage
Copy link
Contributor

@joeyhage joeyhage commented Sep 4, 2025

I added documentation to aws-lambda-builders for its API. I was unable to find the existing BuildProperties documentation to update.

Which issue(s) does this change fix?

#6593

Why is this change necessary?

Python allows ambiguous relative imports so importing uuid is usually from the standard library but could also be a file uuid.py adjacent to the handler module. PEP 8 advises absolute imports or explicit relative imports, and the Google Python Style Guide provides examples of the pitfalls.

How does it address the issue?

This change integrates with a change that was merged in AWS Lambda Builders. It provides a mechanism for explicit package names and auto package names.

What side effects does this change have?

N/A, it is opt-in only.

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.

@github-actions github-actions bot added area/build sam build command pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Sep 4, 2025
@joeyhage joeyhage marked this pull request as ready for review September 4, 2025 15:35
@joeyhage joeyhage requested a review from a team as a code owner September 4, 2025 15:35
@joeyhage
Copy link
Contributor Author

joeyhage commented Sep 4, 2025

Note: PR checks will fail until aws-lambda-builders releases 1.58.0 with aws/aws-lambda-builders#765, but everything works locally using pip editable mode and both repos.

@joeyhage
Copy link
Contributor Author

joeyhage commented Sep 9, 2025

I noticed that aws-lambda-builders 1.58.0 was released so this is ready for PR workflows now. Thank you!

@reedham-aws
Copy link
Contributor

Yes, we released your builders changes so we could take a look at this one. I will follow up soon. Thanks again for your contributions!

@joeyhage
Copy link
Contributor Author

joeyhage commented Sep 9, 2025

I saw some integration tests failed and I have pushed a fix for that issue. I ran the integration tests locally with python 3.12 which is why I didn't see that issue.

For the Update reproducible requirements failure, it looks like a permissions issue since my PR is from a fork.

@joeyhage
Copy link
Contributor Author

joeyhage commented Sep 9, 2025

For the Update reproducible requirements failure, it looks like a permissions issue since my PR is from a fork.

I rebased off the develop branch so my PR no longer includes changes to requirements/base.txt and therefore this workflow will not run again.

Copy link
Contributor

@reedham-aws reedham-aws left a comment

Choose a reason for hiding this comment

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

The code itself looks good to me, nice work @joeyhage!

I don't think there's a place in any of the documentation in this repo that could be updated with this change. Other BuildProperties are documented in official AWS documentation here, so that's something we'd have to take care of internally. I will discuss this internally with the team.

@joeyhage
Copy link
Contributor Author

Thanks, @reedham-aws! I really appreciate your feedback and support with both PRs for this feature.

@vicheey
Copy link
Contributor

vicheey commented Sep 13, 2025

Hi @joeyhage,
Thanks for the great contribution and for being responsive to feedback throughout the review process.
The implementation is clean, well-tested, and properly integrates with existing SAM CLI patterns. I like the dual approach (auto/explicit). Overall look good to me.

Copy link
Contributor

@reedham-aws reedham-aws left a comment

Choose a reason for hiding this comment

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

After discussing, we will merge this and it will be officially released in the next version of SAM CLI. For the documentation, we will handle it, although it's not blocking for the actual release. Thank you again for your contributions @joeyhage!!

@reedham-aws reedham-aws added this pull request to the merge queue Sep 15, 2025
Merged via the queue into aws:develop with commit e319d46 Sep 15, 2025
50 checks passed
@reedham-aws
Copy link
Contributor

@joeyhage
Copy link
Contributor Author

Thanks, @reedham-aws! I noticed a typo in the metadata example on that page. The example used snake case and the code is pascal case. Here's the Sam template used in the tests I wrote: https://github.com/aws/aws-sam-cli/blob/develop/tests/integration/testdata/buildcmd/template_with_metadata_python.yaml#L35

@reedham-aws
Copy link
Contributor

I have alerted this issue to the docs team and provided proper snippets with explanations. Thank you for your prompt response @joeyhage!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/build sam build command pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants