Remove our copy of the wait-for-it script #8572
Conversation
Nowadays it's available from the Ubuntu repositories, so install it from there. This declutters our root directory a bit, plus it makes enumerating 3rd-party dependencies easier.
WalkthroughThe changes primarily involve modifications to the Dockerfile and related scripts to enhance the build process and streamline service dependency checks. The Dockerfile now includes multiple build stages for installing packages, compiling OpenH264 and FFmpeg, and setting up the Smokescreen project. Additionally, the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Backend
participant Database
participant Redis
User->>Backend: Start Application
Backend->>Backend: Check PostgreSQL with wait-for-it
Backend->>Database: Connect to PostgreSQL
Backend->>Backend: Check Redis (in-memory) with wait-for-it
Backend->>Redis: Connect to Redis (in-memory)
Backend->>Backend: Check Redis (on-disk) with wait-for-it
Backend->>Redis: Connect to Redis (on-disk)
Backend->>User: Application Ready
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
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
wait_for_deps.sh (1)
14-16: LGTM! Consider adding comments for improved readability.The changes align well with the PR objectives by removing the internal copy of the
wait-for-itscript and using the system-wide version. The functionality remains the same, which is good.Consider adding brief comments before each
wait-for-itcall to improve readability. For example:+# Wait for PostgreSQL wait-for-it "${CVAT_POSTGRES_HOST}:${CVAT_POSTGRES_PORT:-5432}" -t 0 +# Wait for Redis in-memory wait-for-it "${CVAT_REDIS_INMEM_HOST}:${CVAT_REDIS_INMEM_PORT}" -t 0 +# Wait for Redis on-disk wait-for-it "${CVAT_REDIS_ONDISK_HOST}:${CVAT_REDIS_ONDISK_PORT}" -t 0This small change would make the script's purpose even clearer at a glance.
backend_entrypoint.sh (1)
22-22: LGTM! Consider refactoring for DRY principle.The change from
~/wait-for-it.shtowait-for-itis consistent with the previous modification and aligns with the PR objective.Consider refactoring the
wait-for-itcalls into a separate function to reduce code duplication. For example:wait_for_service() { local host="$1" local port="$2" wait-for-it "${host}:${port}" -t 0 } # Usage in wait_for_db function wait_for_service "${CVAT_POSTGRES_HOST}" "${CVAT_POSTGRES_PORT:-5432}" # Usage in cmd_init function wait_for_service "${CVAT_REDIS_INMEM_HOST}" "${CVAT_REDIS_INMEM_PORT:-6379}"This refactoring would make the script more maintainable and easier to update if changes to the
wait-for-itusage are needed in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- Dockerfile (2 hunks)
- backend_entrypoint.sh (2 hunks)
- wait-for-it.sh (0 hunks)
- wait_for_deps.sh (1 hunks)
💤 Files with no reviewable changes (1)
- wait-for-it.sh
🧰 Additional context used
🔇 Additional comments (4)
wait_for_deps.sh (1)
14-16: Verify system-wide availability ofwait-for-itThe changes assume that
wait-for-itis available in the system PATH. To ensure smooth operation after these changes:
- Confirm that the Dockerfile or the environment where this script runs installs the
wait-for-itpackage.- Verify that the
wait-for-itcommand is available in the PATH when this script is executed.- Check that no other files in the project still reference the old
~/wait-for-it.shscript.Run the following script to verify these points:
backend_entrypoint.sh (1)
Line range hint
1-72: Summary: Successful migration to systemwait-for-itcommand.The changes in this file successfully replace the local
wait-for-it.shscript with the systemwait-for-itcommand. This aligns perfectly with the PR objective of removing the project's internal copy of the wait-for-it script. The modifications maintain the existing functionality while simplifying the project structure and improving dependency management.These changes, combined with the updates in the Dockerfile to install the
wait-for-itpackage, effectively streamline the project setup without compromising its operational integrity.Dockerfile (2)
137-137:⚠️ Potential issueVerify the availability of 'wait-for-it' in Ubuntu 22.04 repositories
You've added
wait-for-itto the list of packages in theapt-getinstall command. Please confirm thatwait-for-itis available in the Ubuntu 22.04 repositories under this package name. If it's not available, the build process may fail.Run the following script to check if
wait-for-itis available:#!/bin/bash # Description: Verify if 'wait-for-it' is available in Ubuntu 22.04 repositories # Update package lists apt-get update # Search for the 'wait-for-it' package apt-cache search '^wait-for-it$'
196-196:⚠️ Potential issueEnsure scripts reference the system-installed 'wait-for-it' command
Since you're no longer copying
wait-for-it.shinto the image, make sure all scripts that previously referenced./wait-for-it.share updated to use thewait-for-itcommand installed by the package manager.Run the following script to find any remaining references to
./wait-for-it.sh:✅ Verification successful
Verification Successful: No references to
./wait-for-it.shfound in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find occurrences of './wait-for-it.sh' in the codebase # Search recursively for 'wait-for-it.sh' references rg './wait-for-it\.sh'Length of output: 118
Script:
#!/bin/bash # Description: Find occurrences of './wait-for-it.sh' in the codebase # Search recursively for 'wait-for-it.sh' references rg './wait-for-it\.sh'Length of output: 24
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8572 +/- ##
===========================================
- Coverage 74.29% 74.26% -0.03%
===========================================
Files 403 403
Lines 43287 43287
Branches 3914 3914
===========================================
- Hits 32159 32149 -10
- Misses 11128 11138 +10
|
Motivation and context
Nowadays it's available from the Ubuntu repositories, so install it from there. This declutters our root directory a bit, plus it makes enumerating 3rd-party dependencies easier.
How has this been tested?
Manual testing.
Checklist
developbranch[ ] I have created a changelog fragment[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)[ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
wait-for-itutility for service readiness checks.Bug Fixes
wait-for-it.shscript with a direct call towait-for-it, ensuring reliable service checks.Chores
Refactor
backend_entrypoint.shandwait_for_deps.shscripts for better clarity and performance.