Note that this approach was once abandoned because it does not work well with larger files (#12903, #18659).

However, I think most use cases for this feature involve small files (e.g., .env), so it's a valuable trade-off imo. We might need something like Source.dataV2 to avoid breaking changes though.

@tmokmss

Note that this approach was once abandoned because it does not work well with larger files (#12903, #18659 ).

However, I think most use cases for this feature involve small files (e.g., .env), so it's a valuable trade-off imo. We might need something like Source.dataV2 to avoid breaking changes though.

• Just to be clear, are you saying that there was a previous implementation that directly passed tokens to markers? If so, where could I find it? I can't find any references to this on the links you provided.
• Likewise, I can't find an explanation on why this is will not work well with larger files.
• I don't see how this can be a breaking change, the assets once deployed will emit the same output as confirmed by the integration tests.

Hi @Abogical, sorry about my incomplete comment! I did a bit of archaeology on this issue before, and I can't resist commenting on it 🥲

As far as I understand, this implementation is essentially the same as the one in the comment #12903 (comment), which passes the entire file content to the custom resource. However, another user claims that this approach has a size limit: #12903 (comment).

This is why the current hacky implementation with marker replacement is used. It should reduce the payload size for the custom resource in many cases.

I guess the current integration tests do not include test cases with large files. However, as long as the file size limit decreases, I think this can be a breaking change.

Hi @Abogical, sorry about my incomplete comment! I did a bit of archaeology on this issue before, and I can't resist commenting on it 🥲

As far as I understand, this implementation is essentially the same as the one in the comment #12903 (comment), which passes the entire file content to the custom resource. However, another user claims that this approach has a size limit: #12903 (comment).

This is why the current hacky implementation with marker replacement is used. It should reduce the payload size for the custom resource in many cases.

I guess the current integration tests do not include test cases with large files. However, as long as the file size limit decreases, I think this can be a breaking change.

Thanks for the clarification @tmokmss . You're right, I can see the case for this when using big json files in a single jsonData call. This PR combines all of the file contents into a single marker if it contains at least one token. Its an edge case that can indeed be a breaking change.

I'll have to modify the PR to handle this. I think a solution to this is for each token to have its own marker, but no token should be resolved regardless before synth.

Thank you for bringing this up! I'll mark this PR as do-not-merge.

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: de08614
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

I think this looks good 👍 It's clear and more straightforward. (not entirelly sure why the hacky approach was initially taken though.)

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.