-
Notifications
You must be signed in to change notification settings - Fork 17
feat: update rust lint and test job #142
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
base: main
Are you sure you want to change the base?
Conversation
… workflow changes
… into SRE-1025/rust-workflow
| COPY ./compliant-reward-distribution/indexer-and-server ./ | ||
| COPY ./deps/concordium-rust-sdk /deps/concordium-rust-sdk | ||
| RUN cargo build --release | ||
| COPY ../ ./ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing it to this will include the front end from the parent folder, do we intend to have the front end copied into the docker env? There is a danger here that anything else added to the parent folder will also be copied into the docker env if someone is not familiar - just noting this incase that part was missed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using workspaces, the entire workspace code is in the temporary builder container, see comment below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when you are doing the copy here, you are copying everything in the parent folder into the baked image right - so when the image runs it will contain all the content from the parent which includes the front end. So although yes the project workspace is available during the build process, you dont want to copy that right as it increases the image size and it has things that are probably not necessary for this particular docker image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No that copy is a copy into the build stage/build container. The copy into the output image is the line
COPY --from=build ./indexer/target/release/crdindexer /crdindexer
Or am I overlooking something? :)
| WORKDIR /server | ||
| COPY ./trackAndTrace/indexer ./ | ||
| RUN cargo build --release | ||
| COPY . . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isnt this copying everything inside the current dockerfiles directory? this seems like it might be incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docker files are being executed in the root of the repo folder (based on the given context).
https://github.com/Concordium/concordium-dapp-examples/blob/main/trackAndTrace/docker-compose.yml
But as you mention the current copying of everything in the root seems not ideal as we want to generate a small image (only with necessary code in it) and there is a potential that something (e.g. .env or e.g. key files AccountAddress.export) gets leaked into the image the more we copy into the image.
In the end, we only publish the final image FROM debian:bookworm which only copied the binary files over from above temporary building image (meaning the final image is small/concise with no .env files or what so ever), but I still feel people might not be fully aware of it and as such would try to keep unnecessary things out of the temporary image as well (in case the code gets changed and such an image gets released for whatever reason).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build stages (temporary containers that builds the build output) exists for the very reason: you don't get build dependencies, whether source code or build chain/libraries, into the final image. So I think we are using best practice here. When using workspaces, you need the source code for the entire repo, it is part of what you buy into with workspaces. But I don't think it is a problem either, copying the files takes very little time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(By the way, I think this PR likely improves what is copied, since before the entire rust sdk submodule, with all its transitive submodules, was copied into the builder stage)
| # It seems as if the `crdindexer` package outputs both and indexer and a server binary... | ||
| # Is the name just a little misleading? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the readme, there should be two binaries (server and indexer) in the package, so I agree that the name is a bit misleading, maybe:
- compliance-reward-distribution-backend
- compliance-reward-distribution-indexer-and-server
If we keep the old name then probably crd-indexer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed such that the crate name and binary names are as before the PR, just with crd- prefixed and applied the change in readme and dockerfiles also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix build of this before merge
… into SRE-1025/rust-workflow
Purpose
Updates the Rust components to share a workspace, and rust-ci.yaml workflow to run a single set of linting/validation/build jobs against all crates.
Also removed the Rust SDK as submodule. The SDK dependencies are now via crates.io