Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
chore: switch docker base image, use system rocksdb for faster compilation #487
chore: switch docker base image, use system rocksdb for faster compilation #487
Changes from 7 commits
88cfb62
4f11a7d
4f8f23f
d1eb5b7
2c0f07a
05d01b0
19f5e48
d3753f2
0bc5f0d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
💡 Codebase verification
Update Required:
protobuf.yml
Still Usesubuntu-latest
The script identified that
.github/workflows/protobuf.yml
is still configured to run onubuntu-latest
. To maintain consistent workflow environments, please update this file to useubuntu-24.04
as well..github/workflows/protobuf.yml
🔗 Analysis chain
Approve the update to Ubuntu 24.04, but verify consistency across workflows
The change from
ubuntu-latest
toubuntu-24.04
is in line with the PR objectives to switch the Docker base image. This update ensures a consistent and predictable environment for the workflow execution.To ensure consistency across all workflow files, please run the following script:
If the script returns any results, consider updating those files to use
ubuntu-24.04
as well for consistency.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 118
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.
LGTM with a minor correction: Clear configuration instructions
The instructions for configuring the build environment using either
.cargo/config.toml
or environment variables are clear and provide flexibility for different setups. The note about default values in.cargo/config.toml
is helpful for users to understand the existing configuration.There's a minor typo in the comment on line 379. It should be "do" instead of "to". Please apply the following change:
🧰 Tools
🪛 LanguageTool
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.
Consider using a stable Ubuntu LTS base image
Switching the base image to
ubuntu:noble
might introduce instability ifnoble
is not a long-term support (LTS) release or is still under development. For enhanced stability and support, consider using an LTS version likeubuntu:22.04
.Apply this diff to change the base image to an LTS version:
📝 Committable suggestion
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.
🛠️ Refactor suggestion
Ensure Rust and its environment are properly installed
Manually installing Rust using the curl script requires careful handling to set up the environment correctly. To ensure that Rust is available in subsequent commands without sourcing the environment every time, consider adding Rust to the PATH globally.
Apply this diff to update the Rust installation:
This change adds the Rust environment to the
.bashrc
file, ensuring that it's loaded in all subsequent steps.📝 Committable suggestion
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.
🛠️ Refactor suggestion
Simplify the Cargo build command
Avoid hardcoding the path to the Cargo binary. Since Rust's bin directory is added to the PATH, you can invoke
cargo
directly.Apply this diff to simplify the build command:
📝 Committable suggestion
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.
Align runtime base image with a stable release
Using
ubuntu:noble
as the runtime base image may lead to compatibility issues ifnoble
is not an officially released or stable version. For consistency and reliability, consider using the same stable LTS version as in the builder stage.Apply this diff to update the runtime base image:
📝 Committable suggestion