Updated ahoy db to use a consistent way to get the DB port. #1884
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe update modifies the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: in the vortex project, unauthenticated composer installs should be allowed, so github token secrets ...Applied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
.vortex/installer/tests/Fixtures/install/_baseline/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (2)
.ahoy.yml(2 hunks).vortex/installer/src/Prompts/Handlers/Services.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: do not review files in `.vortex/installer/tests/fixtures/install` directory as they are test fixture...
Learnt from: AlexSkrypnyk
PR: drevops/vortex#0
File: :0-0
Timestamp: 2025-05-29T12:15:32.188Z
Learning: Do not review files in `.vortex/installer/tests/Fixtures/install` directory as they are test fixtures.
Applied to files:
.vortex/installer/src/Prompts/Handlers/Services.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-docs
🔇 Additional comments (1)
.ahoy.yml (1)
133-137: Formatting tweak acknowledgedIndentation/line-continuation adjustments only; behaviour is unchanged and still coherent with the surrounding chain.
Comment on lines
46
to
48
| uname -a | grep -i -q darwin && mdfind -name 'Sequel Ace' 2>/dev/null |grep -q "Ace" && \ | ||
| VORTEX_HOST_DB_PORT=$(docker compose port database 3306 2>/dev/null | cut -d : -f 2) && \ | ||
| open "mysql://${DATABASE_USERNAME:-drupal}:${DATABASE_PASSWORD:-drupal}@127.0.0.1:${VORTEX_HOST_DB_PORT}/drupal" -a "Sequel Ace" \ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against empty / multi-line docker compose port output before launching Sequel Ace
docker compose port … 3306 may return zero or multiple lines (IPv4 & IPv6).
If it returns nothing (e.g., database container not running) open will still execute with an empty port, succeed (exit 0) and silently skip the fallback message.
Capturing the first line and validating the port avoids this silent failure.
- VORTEX_HOST_DB_PORT=$(docker compose port database 3306 2>/dev/null | cut -d : -f 2) && \
- open "mysql://${DATABASE_USERNAME:-drupal}:${DATABASE_PASSWORD:-drupal}@127.0.0.1:${VORTEX_HOST_DB_PORT}/drupal" -a "Sequel Ace" \
+ VORTEX_HOST_DB_PORT=$(docker compose port database 3306 2>/dev/null | head -n1 | cut -d : -f 2) && \
+ if [ -n "${VORTEX_HOST_DB_PORT}" ]; then \
+ open "mysql://${DATABASE_USERNAME:-drupal}:${DATABASE_PASSWORD:-drupal}@127.0.0.1:${VORTEX_HOST_DB_PORT}/drupal" -a "Sequel Ace"; \
+ else \
+ echo "Database container is not running – unable to determine host port."; \
+ fi \🤖 Prompt for AI Agents
In the .ahoy.yml file around lines 46 to 48, the command uses `docker compose
port database 3306` which can return empty or multiple lines, causing the open
command to run with an invalid or empty port. To fix this, capture only the
first line of the port output, check if it is non-empty and valid before
proceeding to run the open command. Add a guard condition to ensure the port
variable is set and valid to prevent silent failures when the database container
is not running or multiple ports are returned.
Comment on lines
129
to
131
| File::removeLine($t . DIRECTORY_SEPARATOR . '.ahoy.yml', 'VORTEX_HOST_SOLR_PORT=$(docker compose port solr 8983 2>/dev/null | cut -d : -f 2) && \\'); | ||
| // @todo Remove after 25.10.0 release. | ||
| File::removeLine($t . DIRECTORY_SEPARATOR . '.ahoy.yml', 'VORTEX_HOST_SOLR_PORT=$(docker compose port solr 8983 2>/dev/null | cut -d : -f 2) \\'); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Duplicate literal patterns – consider a single regex
Two almost-identical File::removeLine() calls differ only by the trailing && \.
Using one regex (or trimming whitespace before comparison) would be more future-proof and avoid drift if the line changes again.
File::removeLine(
$t . DIRECTORY_SEPARATOR . '.ahoy.yml',
'/VORTEX_HOST_SOLR_PORT=\$\(docker compose port solr 8983 .* cut -d : -f 2\).*\\\\?/'
);🤖 Prompt for AI Agents
In .vortex/installer/src/Prompts/Handlers/Services.php around lines 129 to 131,
there are two File::removeLine calls with nearly identical string patterns
differing only by a trailing '&& \'. Replace these two calls with a single
File::removeLine call using a regex pattern that matches both variants, such as
one that matches the line starting with 'VORTEX_HOST_SOLR_PORT=$(docker compose
port solr 8983' and optionally ends with '&& \' or '\'. This will make the code
more maintainable and prevent issues if the line changes in the future.
github-actions
bot
added
the
CONFLICT
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1884 +/- ##
========================================
Coverage 78.37% 78.37%
========================================
Files 85 85
Lines 5003 5003
Branches 35 35
========================================
Hits 3921 3921
Misses 1082 1082 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AlexSkrypnyk
force-pushed
the
feature/ahoy-db-consistent-port
branch
from
cee401d to
7585b22
Compare
github-actions
bot
removed
the
CONFLICT
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by CodeRabbit