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

refactor: swap rust function #152

Merged
merged 4 commits into from
Mar 14, 2024
Merged

refactor: swap rust function #152

merged 4 commits into from
Mar 14, 2024

Conversation

mmalenic
Copy link
Member

@mmalenic mmalenic commented Mar 14, 2024

Closes #133. I think this fixes a few issues around building Rust code.

Changes

  • Swap the aws-cdk-lambda for cargo-lambda-cdk
  • Filemanager is now compiled inside Docker, so CodeBuild no longer requires installing Rust and cargo-lambda.
  • Importantly, the filemanager Lambda function is still deployed natively using a cargo-lambda, only the compilation occurs inside Docker.
  • No more auto-generated query-* files.

These changes should make it easier to build the Filemanager code without special dependencies. There is a small downside in that the docker container produces code which isn't owned by the current user, meaning that sometimes permissions errors can occur if trying to remove/clean this directory. Although, I've documented this.

…st-function

# Conflicts:
#	lib/workload/stateless/filemanager/.sqlx/query-63da484d18ed2571639cfddadf992fe9ea92767a8274859de371f4e954e554b8.json
#	lib/workload/stateless/filemanager/.sqlx/query-7495c606d6e756a6e53e45fc0ee6a0765922a5fef3d512c719d4aec543d0d8a6.json
#	lib/workload/stateless/filemanager/.sqlx/query-c05597a449cf3e2e125bec198486d2bc57b270abf737f64cfe386650fd1d2b9e.json
@mmalenic mmalenic force-pushed the refactor/swap-rust-function branch from a44a62f to eb3654e Compare March 14, 2024 01:53
@mmalenic mmalenic self-assigned this Mar 14, 2024
@@ -29,10 +29,10 @@
"@aws-cdk/aws-apigatewayv2-integrations-alpha": "^2.110.0-alpha.0",
"@aws-cdk/aws-lambda-python-alpha": "2.110.0-alpha.0",
"aws-cdk-lib": "^2.126.0",
"cargo-lambda-cdk": "^0.0.19",
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm wondering if this should go in a package.json inside the filemanager directory? I noticed @williamputraintan did this for the postgres manager.

Copy link
Member

@williamputraintan williamputraintan Mar 14, 2024

Choose a reason for hiding this comment

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

But will the pipeline environment still have access to install the dependency as part of the CDK? The postgres manager package.json but its for the lambda runtime

Copy link
Member

Choose a reason for hiding this comment

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

I think, cargo-lambda-cdk here is fine. It is part of the cdk build time or rather cdk synthesis requirement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, sounds good. One approach could be to have multiple yarn workspaces as well? Although I think for now that's probably unnecessary for one dependency.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, yup. Well spotted, Marko. We're gonna hit the yarn workspaces in tick, real soon. Let sit tight for the moment, pls.

@mmalenic mmalenic added filemanager an issue relating to the filemanager fix labels Mar 14, 2024
Copy link
Member

@victorskl victorskl left a comment

Choose a reason for hiding this comment

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

Nicely done.! I just tested locally and it works well.

@victorskl
Copy link
Member

It builds twice for each function though - migrate and ingest. But I reckon that is fine. We can optimise those bits later.

@mmalenic
Copy link
Member Author

mmalenic commented Mar 14, 2024

It builds twice for each function though - migrate and ingest. But I reckon that is fine. We can optimise those bits later.

Hmm, good point, that might slow things down a bit as there's definitely overlapping code (i.e. filemanager crate). I'll look into this.

@mmalenic mmalenic merged commit f2329f3 into main Mar 14, 2024
2 checks passed
@mmalenic mmalenic deleted the refactor/swap-rust-function branch March 14, 2024 05:14
@victorskl
Copy link
Member

No rush. Early days...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filemanager an issue relating to the filemanager fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

filemanager: consider using cargo-lambda-cdk instead of aws-cdk-lambda
3 participants