Skip to content
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

fix(injector: redigo) fix bad argument type from []redis.DialOption to []any #351

Merged
merged 7 commits into from
Oct 25, 2024

Conversation

eliottness
Copy link
Contributor

@eliottness eliottness commented Oct 22, 2024

Error:

❯ orchestrion go test -tags dynamic -args integration -v ./... -count 1
# go.ddbuild.io/dd-source/x/libs/go/redis
/Users/tony.redondo/go/pkg/mod/go.ddbuild.io/dd-source/[email protected]/libs/go/redis/connector.go:38: cannot use dialOptions (variable of type []"github.com/gomodule/redigo/redis".DialOption) as []interface{} value in argument to __orchestrion_redigo.Dial
exit status 2
FAIL    github.com/DataDog/dd-go/ci-app/apps/ci-test-commit-summarizer/testsummarizer [build failed]

Fix github.com/gomodule/redigo/redis aspects where adding options ahead of making calls would fail the build because of the differences of signatures between the original and replaced function:

Original DialURL function:

func DialURL(rawurl string, options ...redis.DialOption) (redis.Conn, error)

dd-trace-go DialURL function:

func DialURL(rawurl string, options ...any) (redis.Conn, error)

Therefore we need to cast the args before sending them to dd-trace-go

@eliottness eliottness force-pushed the eliott.bouhana/cast-redigo-optiosn branch from c7d5bdb to c408cea Compare October 22, 2024 16:26
@eliottness eliottness changed the title fix(injectoe: redigo) fix bad argument type from []redigo.DialOption to []any fix(injector: redigo) fix bad argument type from []redigo.DialOption to []any Oct 22, 2024
@eliottness eliottness changed the title fix(injector: redigo) fix bad argument type from []redigo.DialOption to []any fix(injector: redigo) fix bad argument type from []redis.DialOption to []any Oct 22, 2024
@eliottness eliottness marked this pull request as ready for review October 22, 2024 16:27
@eliottness eliottness requested a review from a team as a code owner October 22, 2024 16:27
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
@RomainMuller RomainMuller added this pull request to the merge queue Oct 25, 2024
Merged via the queue into main with commit 7d24aff Oct 25, 2024
23 checks passed
@RomainMuller RomainMuller deleted the eliott.bouhana/cast-redigo-optiosn branch October 25, 2024 10:46
Copy link

codecov bot commented Oct 25, 2024

Codecov Report

Attention: Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 57.91%. Comparing base (80295ab) to head (1910bb1).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
_integration-tests/tests/redigo/redigo.go 62.50% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #351      +/-   ##
==========================================
- Coverage   61.06%   57.91%   -3.15%     
==========================================
  Files         151      153       +2     
  Lines        9544    11124    +1580     
==========================================
+ Hits         5828     6443     +615     
- Misses       3246     4213     +967     
+ Partials      470      468       -2     
Components Coverage Δ
Generators 76.98% <ø> (-1.41%) ⬇️
Instruments 88.05% <ø> (ø)
Go Driver 72.81% <ø> (-7.44%) ⬇️
Toolexec Driver 70.88% <ø> (-3.42%) ⬇️
Aspects 70.85% <ø> (-6.27%) ⬇️
Injector 73.14% <ø> (-4.22%) ⬇️
Job Server 64.02% <ø> (-5.80%) ⬇️
Integration Test Suite 48.27% <62.50%> (-2.07%) ⬇️
Other 57.91% <62.50%> (-3.15%) ⬇️
Files with missing lines Coverage Δ
_integration-tests/tests/redigo/redigo.go 81.13% <62.50%> (-8.76%) ⬇️

... and 132 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants