Skip to content

CI/CD fixes #994

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 27, 2025
Merged

CI/CD fixes #994

merged 2 commits into from
May 27, 2025

Conversation

jlizen
Copy link
Contributor

@jlizen jlizen commented May 16, 2025 •
edited
Loading

📬 Issue #, if available:
n/a

✍️ Description of changes:

Build integration tests step failing its unit test
https://github.com/awslabs/aws-lambda-rust-runtime/actions/runs/14964429921/job/42031762957

thread 'test_calling_lambda_should_return_200' panicked at lambda-integration-tests/tests/integration_test.rs:11:5:
assertion `left == right` failed
  left: 502
 right: 200
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

For this one, we don't want to actually run the test during non-integ-test workflow runs, just build the package. Tweaking the workflow config to accomplish that.

Clippy failure due to large enum variant
https://github.com/jlizen/aws-lambda-rust-runtime/actions/runs/15074705748/job/42379333173

31 | /  pub enum RuntimeApiResponseFuture<F, Response, BufferedResponse, StreamingResponse, StreamItem, StreamError> {
132 | |/     Future(
133 | ||         #[pin] F,
134 | ||         String,
135 | ||         PhantomData<(
...   ||
142 | ||         )>,
143 | ||     ),
    | ||_____- the second-largest variant contains at least 24 bytes
144 | |      Ready(Option<Result<http::Request<Body>, BoxError>>),
    | |      ---------------------------------------------------- the largest variant contains at least 240 bytes
145 | |  }
    | |__^ the entire enum is at least 240 bytes
    |

We have what is essentially a handrolled futures::future::MaybeDone in our runtime's tower service. We need this in order to model the case where we can't call the inner service at all because of a failure to process the lambda invocation into a lambda event, in which case the future is immediately ready, and then its output is fairly large since it is handing the caller a new http request to make to the runtime.

I think boxing is probably the right choice here, to avoid the error handling path bloating the happy path's variant to be 10x the size it needs to be. Most users would presumably prefer their happy path to be a bit more efficient at the cost of an extra allocation on an unhappy path that should be infrequent and will anyway fail the overall invocation (and possibly kill their runtime).

Technically MaybeDone would be slightly more efficient by saving us an Option, but it so trivial that it's not worth the refactor.

🔏 By submitting this pull request

  • I confirm that I've ran cargo +nightly fmt.
  • I confirm that I've ran cargo clippy --fix.
  • I confirm that I've made a best effort attempt to update all relevant documentation.
  • I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jlizen
Copy link
Contributor Author

jlizen commented May 16, 2025 •
edited
Loading

@bnusunny this should fix one of the ci/cd failures. i'm not able to reproduce the clippy failure locally - mind re-triggering workflows so i can validate whether it remains an issue?

EDIT: Nevermind, integ tests are running on my fork, so I can validate the clippy one there (still an issue).

The integ test one though will need to run here to validate.

@jlizen jlizen marked this pull request as ready for review May 16, 2025 19:34
@jlizen
Copy link
Contributor Author

jlizen commented May 16, 2025

@bnusunny clippy is happy on my branch now, and i'm pretty confident that the patch to the build-integration-tests workflow will work fine. Though won't know until we run workflows here.

@jlizen jlizen changed the title (WIP) CI/CD test fixes CI/CD fixes May 16, 2025
@bnusunny
Copy link
Contributor

Hmm, build integration tests check failed because it tried to run integration tests and the integration test stack was not deployed. The build integratio tests check should not run integration tests. I need to take a deeper look at these GitHub Actions.

@jlizen
Copy link
Contributor Author

jlizen commented May 19, 2025

Hmm, build integration tests check failed because it tried to run integration tests and the integration test stack was not deployed. The build integratio tests check should not run integration tests. I need to take a deeper look at these GitHub Actions.

Hmm, I tried to address that like so:
ef84ee0

But it doesn't seem to have kicked in. I'll poke around more this week. Looks like I can repro the issue in my own branch.

The idea would be, we want to build the integration tests binary, but not execute the tests.

@jlizen
Copy link
Contributor Author

jlizen commented May 27, 2025 •
edited
Loading

@bnusunny the build integration tests step is passing now on my fork, it is properly building the binary to validate that it compiles, without running the e2e tests that are failing. test run link

TIL you need:

      if: ${{ inputs.run-tests == 'true' }}

rather than:

      if: ${{ inputs.run-tests }}

I've also confirmed that the tests are still running for other workflows that are using the run-tests input's default value of true. Example.

bnusunny
bnusunny approved these changes May 27, 2025
View reviewed changes
Copy link
Contributor

@bnusunny bnusunny left a comment

Choose a reason for hiding this comment

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

LGTM

@bnusunny bnusunny merged commit 489b235 into awslabs:main May 27, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnusunny bnusunny bnusunny approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jlizen @bnusunny