chore!: wait.ForSQL: change url callback to accept network.Network#3650
Open
thaJeztah wants to merge 1 commit intotestcontainers:mainfrom
Open
chore!: wait.ForSQL: change url callback to accept network.Network#3650thaJeztah wants to merge 1 commit intotestcontainers:mainfrom
thaJeztah wants to merge 1 commit intotestcontainers:mainfrom
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThis PR changes SQL port handling to use the typed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thaJeztah
force-pushed
the
wait_sql_signature
branch
from
992afa3 to
a979eb2
Compare
thaJeztah
changed the title
Contributor
Author
|
Are that conventional commit check is picky; looks like it doesn't support "scope"? https://www.conventionalcommits.org/en/v1.0.0/#commit-message-with-scope-and--to-draw-attention-to-breaking-change
|
thaJeztah
force-pushed
the
wait_sql_signature
branch
from
a979eb2 to
415c99f
Compare
thaJeztah
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/cockroachdb/cockroachdb.go (1)
123-123:⚠️ Potential issue | 🔴 CriticalType mismatch: passing
stringwherenetwork.Portis expected.Line 123 calls
c.connConfig(host, port.String()), but the updatedconnConfigsignature at line 238 now expectsport network.Port. This will cause a compilation error.Since
portis already anetwork.PortfromMappedPort, pass it directly without conversion.🐛 Proposed fix
- return c.connConfig(host, port.String()) + return c.connConfig(host, port)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/cockroachdb/cockroachdb.go` at line 123, The call to connConfig is passing a string but connConfig now expects a network.Port; update the call at the site using the MappedPort variable (currently named port) to pass it directly (i.e., pass port instead of port.String()) so the types match; locate the call to c.connConfig(...) in cockroachdb.go and replace the port.String() argument with the port MappedPort variable itself to satisfy the connConfig(host, port network.Port) signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wait/sql_test.go`:
- Around line 56-58: The PrepareContext method on mockSQLConn has the wrong
second parameter type; change mockSQLConn.PrepareContext to match
database/sql/driver.ConnPrepareContext by accepting (ctx context.Context, query
string) and returning (driver.Stmt, error) — keep returning &mockSQLStmt{}, nil
but update the parameter name/type from network.Port to string so the signature
matches the interface.
- Around line 42-44: The mock implementations use the wrong parameter types due
to an accidental refactor: change mockSQLDriver.Open to match
database/sql/driver.Driver by taking a string (Open(name string) (driver.Conn,
error)) instead of network.Port, and change mockSQLConn.PrepareContext to match
database/sql/driver.ConnPrepareContext by taking (ctx context.Context, query
string) and returning (driver.Stmt, error) instead of using network.Port; update
the function signatures and any call sites or tests in this file to use the
correct types and imports (context and database/sql/driver) and remove the
incorrect network.Port parameter usage.
---
Outside diff comments:
In `@modules/cockroachdb/cockroachdb.go`:
- Line 123: The call to connConfig is passing a string but connConfig now
expects a network.Port; update the call at the site using the MappedPort
variable (currently named port) to pass it directly (i.e., pass port instead of
port.String()) so the types match; locate the call to c.connConfig(...) in
cockroachdb.go and replace the port.String() argument with the port MappedPort
variable itself to satisfy the connConfig(host, port network.Port) signature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 782bd351-070c-424a-81ba-8de4dc8bc870
📒 Files selected for processing (4)
modules/cockroachdb/cockroachdb.gomodules/postgres/postgres_test.gowait/sql.gowait/sql_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/cockroachdb/cockroachdb.go`:
- Around line 238-239: The caller of CockroachDBContainer.connConfig is still
passing port.String(), causing a type mismatch after connConfig now expects
network.Port; locate the call site that invokes connConfig (the call that
currently passes port.String()) and remove the .String() so the raw network.Port
value is passed instead, ensuring types align with connConfig(host string, port
network.Port) and update any intermediate variables to use network.Port rather
than string if necessary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 438a1efd-165e-432e-8697-63928658867a
📒 Files selected for processing (4)
modules/cockroachdb/cockroachdb.gomodules/postgres/postgres_test.gowait/sql.gowait/sql_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- modules/postgres/postgres_test.go
- wait/sql.go
- wait/sql_test.go
thaJeztah
force-pushed
the
wait_sql_signature
branch
from
415c99f to
aa620fe
Compare
Commit 3e56fb9 updated this code to use the moby modules, and changed signatures to accept a string for ports ("<port>[/<protocol>]"), handling conversion to strong types (`network.Port`) internally. In this case, however, the url callback function gets passed the host and port that's already parsed internally, so downgrading the parsed `network.Port` to a string means that a custom callback would have to parse the string again. Change it to a strong-typed `network.Port`, which keeps all options open; callbacks that want to use the port in its string-format can call `port.String()`, but if only the port or proto is needed, they can use the `port.Port()`, `port.Proto()` or other methods. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
wait_sql_signature
branch
from
aa620fe to
88fdff7
Compare
|
Thank you for this!! 🙌 |
Contributor
Author
|
cc @mdelapenya PTAL 😄 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Commit 3e56fb9 updated this code to use the moby modules, and changed signatures to accept a string for ports ("
<port>[/<protocol>]"), handling conversion to strong types (network.Port) internally.In this case, however, the url callback function gets passed the host and port that's already parsed internally, so downgrading the parsed
network.Portto a string means that a custom callback would have to parse the string again.Change it to a strong-typed
network.Port, which keeps all options open; callbacks that want to use the port in its string-format can callport.String(), but if only the port or proto is needed, they can use theport.Port(),port.Proto()or other methods.What does this PR do?
Why is it important?
Related issues