chore!: clean up tx sim docker usage #3902
Conversation
cmwaters
requested review from
rootulp and
ninabarbakadze
and removed request for
a team
📝 WalkthroughWalkthroughThe pull request introduces several changes across multiple files, primarily focusing on the restructuring of Dockerfile paths and the introduction of a new entry point script for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
docker/txsim/Dockerfile (1)
Line range hint
1-22: LGTM! Good updates to focus on txsim and improve dependencies.The changes in this segment align well with the PR objectives. The shift from 'celestia-appd' to 'txsim' is consistent, and the updates to Go (1.22.6) and Alpine (3.19) versions are good practices for security and performance.
Consider adding a comment explaining the purpose of the
txsimbinary for better documentation. For example:# Stage 1: generate txsim binary +# txsim is used for transaction simulation in the Celestia network FROM --platform=$BUILDPLATFORM docker.io/golang:1.22.6-alpine3.19 as buildertest/e2e/testnet/txsimNode.go (1)
69-74: Improved clarity with descriptive flag namesThe changes to the argument formatting are a significant improvement. The use of long, descriptive flag names (
-grpc-endpoint,-poll-time,-blob,-blob-amounts,-blob-sizes) enhances code readability and maintainability. This aligns well with the PR objective of cleaning up flag usage and addressing indirection.Minor formatting suggestion for consistency
For consistency, consider removing the space after the value in the
-seedflag argument:- fmt.Sprintf("-seed %d ", seed), + fmt.Sprintf("-seed %d", seed),This would make it consistent with the formatting of the other flag arguments.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- .github/workflows/docker-build-publish.yml (2 hunks)
- .github/workflows/lint.yml (1 hunks)
- Makefile (3 hunks)
- docker/txsim.sh (0 hunks)
- docker/txsim/Dockerfile (3 hunks)
- docker/txsim/entrypoint.sh (1 hunks)
- test/e2e/testnet/txsimNode.go (1 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- docker/txsim.sh
🧰 Additional context used
Shellcheck
docker/txsim/entrypoint.sh
[error] 4-4: Argument mixes string and array. Use * or separate argument.
(SC2145)
[error] 7-7: Double quote array expansions to avoid re-splitting elements.
(SC2068)
🔇 Additional comments not posted (13)
docker/txsim/entrypoint.sh (2)
1-2: LGTM: Proper shebang and good use of whitespace.The shebang is correctly set for a Bash script, and the empty line improves readability.
1-7: LGTM: Entrypoint script is well-structured.This entrypoint script effectively logs the command being executed and then runs the
txsimbinary with all provided arguments. It serves its purpose well as a Docker entrypoint.After applying the suggested fixes for argument handling, the script will be robust and handle all input scenarios correctly.
🧰 Tools
Shellcheck
[error] 4-4: Argument mixes string and array. Use * or separate argument.
(SC2145)
[error] 7-7: Double quote array expansions to avoid re-splitting elements.
(SC2068)
.github/workflows/docker-build-publish.yml (2)
36-36: LGTM! Txsim Dockerfile path updated correctly.The change from
docker/Dockerfile_txsimtodocker/txsim/Dockerfilealigns with the PR objective of moving the txsim Dockerfile to a separate subdirectory. This further improves the project's organization and follows a more conventional naming pattern for Dockerfiles.Let's verify if this change is consistent across the repository:
#!/bin/bash # Description: Check for any remaining references to the old txsim Dockerfile path and confirm the new path # Test: Search for references to the old txsim Dockerfile path echo "Searching for old txsim Dockerfile references:" rg --type yaml --type dockerfile 'Dockerfile_txsim' -l # Test: Confirm the existence of the new txsim Dockerfile echo "Checking if the new txsim Dockerfile exists:" ls docker/txsim/Dockerfile
26-26: LGTM! Dockerfile path updated correctly.The change from
Dockerfiletodocker/Dockerfilealigns with the PR objective of relocating Dockerfiles to a dedicateddockerdirectory. This improves the project's organization.Let's verify if this change is consistent across the repository:
docker/txsim/Dockerfile (4)
Line range hint
24-41: LGTM! Good security practices and environment setup.The update to Alpine 3.20 is a good practice for security and performance. The user creation with a high UID (10001) is an excellent security measure, preventing potential UID conflicts and reducing security risks. The environment setup appears appropriate for the application.
47-48: LGTM! Correct update to copy the txsim binary.The change from copying 'celestia-appd' to 'txsim' is consistent with the PR objectives and the changes made in the build stage. The path and destination of the copy command are correct.
50-50: LGTM! Good update to entrypoint script location and naming.The change from 'txsim.sh' to 'entrypoint.sh' and the updated path reflecting the new docker/txsim directory structure are consistent with the PR objectives. The new naming convention is more standard and clear. The ownership is correctly set to the celestia user, maintaining proper security practices.
Line range hint
52-59: LGTM! Final configuration looks good, but clarification needed on network requirements.The user setup, working directory, and entrypoint configuration remain correct and follow good practices.
However, I noticed that the EXPOSE instruction for ports has been removed. Could you please clarify if this is intentional and if there are any changes to the network requirements for the txsim container? If the container no longer needs to expose any ports, this change is fine. Otherwise, we might need to add back the necessary EXPOSE instructions.
To verify the network requirements, you can run the following command:
✅ Verification successful
Verified: EXPOSE instruction removal is acceptable.
The removal of the
EXPOSEinstruction in thedocker/txsim/Dockerfiledoes not impact thetxsimcontainer's functionality. The shell script outputs indicate no network-related dependencies necessitating exposed ports fortxsim. Therefore, this change is intentional and aligns with the current network requirements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any network-related configurations in the txsim code or documentation # Search for port or network-related configurations in txsim code rg --type go -i '(port|listen|addr|host|network)' $(fd -t f -e go . | grep -i 'txsim') # Check if there are any mentions of network configurations in README or documentation rg -i '(port|listen|addr|host|network)' $(fd -t f -e md . | grep -i 'readme\|doc')Length of output: 65002
Makefile (5)
86-86: LGTM: Dockerfile path updated correctly.The change to use
docker/Dockerfilealigns with the PR objective of relocating Dockerfiles to a dedicateddockerdirectory. This improves the project's organization.
92-92: LGTM: Consistent Dockerfile path update.The change to use
docker/Dockerfilein thebuild-ghcr-dockertarget is consistent with the previous modification and maintains uniformity across Docker-related targets.
109-110: LGTM: Dockerfile linting updated and expanded.The changes in the
linttarget are appropriate:
- The main Dockerfile path has been correctly updated to
docker/Dockerfile.- The addition of linting for
docker/txsim/Dockerfileis a good practice, ensuring code quality for all Docker configurations.These modifications align well with the PR objectives and improve overall project consistency.
Line range hint
86-190: Overall assessment: Docker-related changes improve project organization.The modifications to Dockerfile paths across various targets in the Makefile (
build-docker,build-ghcr-docker,lint, andtxsim-build-docker) consistently reflect the relocation of Dockerfiles to thedockerdirectory and thedocker/txsimsubdirectory. These changes align well with the PR objectives and enhance the project's directory structure.Key improvements:
- Consistent use of
docker/Dockerfilefor the main Dockerfile.- Addition of linting for the txsim Dockerfile.
- Proper relocation of the txsim Dockerfile to
docker/txsim/Dockerfile.These changes contribute to better organization and maintainability of the project's Docker-related components.
190-190: LGTM: txsim Dockerfile path updated correctly.The change to use
docker/txsim/Dockerfilealigns with the PR objective of relocating the txsim Dockerfile to a separate subdirectory. This improves the organization of Docker-related files.Let's verify the existence of the new Dockerfile:
✅ Verification successful
Verified: txsim Dockerfile exists at the updated path.
The Dockerfile has been successfully relocated to
docker/txsim/Dockerfile, aligning with the PR objectives and improving project organization.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of the new txsim Dockerfile if [ -f "docker/txsim/Dockerfile" ]; then echo "docker/txsim/Dockerfile exists." else echo "Error: docker/txsim/Dockerfile not found." exit 1 fiLength of output: 109
| echo "/bin/txsim $@" | ||
| echo "" | ||
|
|
||
| exec /bin/txsim $@ |
There was a problem hiding this comment.
Quote arguments in exec command.
The current usage of $@ in the exec command may lead to unexpected behavior if arguments contain spaces or special characters.
Apply this diff to fix the argument handling:
-exec /bin/txsim $@
+exec /bin/txsim "$@"This change ensures that all arguments are properly passed to the txsim binary, preserving spaces and special characters.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| exec /bin/txsim $@ | |
| exec /bin/txsim "$@" |
🧰 Tools
Shellcheck
[error] 7-7: Double quote array expansions to avoid re-splitting elements.
(SC2068)
| echo "Starting txsim with command:" | ||
| echo "/bin/txsim $@" | ||
| echo "" | ||
|
|
There was a problem hiding this comment.
Fix argument handling in echo statement.
The current usage of $@ in the echo statement may lead to unexpected behavior if arguments contain spaces or special characters.
Apply this diff to fix the argument handling:
-echo "/bin/txsim $@"
+echo "/bin/txsim $*"This change ensures that all arguments are properly represented as a single string when echoed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "Starting txsim with command:" | |
| echo "/bin/txsim $@" | |
| echo "" | |
| echo "Starting txsim with command:" | |
| echo "/bin/txsim $*" | |
| echo "" |
🧰 Tools
Shellcheck
[error] 4-4: Argument mixes string and array. Use * or separate argument.
(SC2145)
| with: | ||
| dockerfile: "docker/Dockerfile" |
There was a problem hiding this comment.
💡 Codebase verification
Additional Dockerfile Detected for Linting
The workflow currently lints docker/Dockerfile. However, an additional Dockerfile docker/txsim/Dockerfile was found. To ensure comprehensive linting, please update the workflow to include this Dockerfile.
- Update the
hadolintjob to includedocker/txsim/Dockerfile. - Consider using a loop or multiple
withclauses if supported by the workflow configuration.
🔗 Analysis chain
LGTM! Consider linting additional Dockerfiles.
The change correctly updates the Dockerfile path for the hadolint job, aligning with the PR objective of relocating Dockerfiles to a dedicated docker directory. This ensures that the linting process targets the correct Dockerfile after the relocation.
To ensure comprehensive linting coverage, please verify if there are other Dockerfiles (such as the one for txsim) that also need linting. If so, consider adding them to this workflow or creating separate linting jobs for them.
Run the following script to check for additional Dockerfiles:
If additional Dockerfiles are found, consider updating the workflow to lint them as well.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all Dockerfiles in the repository
# Test: Search for Dockerfiles. Expect: List of all Dockerfiles in the repository.
fd Dockerfile
Length of output: 57
This PR tidies up our docker usage. Namely, it puts our two Dockerfiles in the
dockerdirectory, and the txsim one separately in thedocker/txsimfolder. It also cleans up the indirection with the flags so our e2e tests can pass them directly.