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

Creates a new flagset to avoid conflit with other libraries that use the same flag #213

Merged
merged 1 commit into from
Dec 12, 2022

Conversation

danimaribeiro
Copy link
Contributor

…the same flag

DataDog/dd-trace-go#1153

❓ What

Baker library conflicts with dd-trace-go library because of dependencies on glog.
glog also defines the flag v
panic: /var/folders/yg/7bk34drs0zjfsb9b_r_1sykr0000gn/T/go-build2940344100/b001/exe/main flag redefined: v

This PR make possible to use both libraries side by side.
It's not possible to send PR to glog because it's a Google internal library that does not accept contributions. But dd-trace-go and also ristretto have dependencies on it.

🔨 How to reproduce the error

Try to import both libraries at the same time:

package main

import (
	"fmt"

	"github.com/AdRoll/baker"
	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
)
func main() {
	tracer.Start(
		tracer.WithEnv("testing"),
		tracer.WithService("tags-automation"),
	)
	defer tracer.Stop()
	components := baker.Components{}
	if err := baker.MainCLI(components); err != nil {
		fmt.Println(err.Error())
	}
}

produces the error:

panic: /var/folders/yn/hdtb3wnd5xg31ygy3vt2g99w0000gn/T/go-build2429850215/b001/exe/testing flag redefined: v

goroutine 1 [running]:
flag.(*FlagSet).Var(0x140000bc120, {0x1059befb8, 0x140004814a8}, {0x1053ac54c, 0x1}, {0x10540798b, 0x1d})
        /usr/local/go/src/flag/flag.go:980 +0x2a4
flag.(*FlagSet).BoolVar(...)
        /usr/local/go/src/flag/flag.go:715
flag.(*FlagSet).Bool(0x0?, {0x1053ac54c, 0x1}, 0x0, {0x10540798b, 0x1d})
        /usr/local/go/src/flag/flag.go:728 +0x70
flag.Bool(...)
        /usr/local/go/src/flag/flag.go:735
github.com/AdRoll/baker.MainCLI({{0x0, 0x0, 0x0}, {0x0, 0x0, 0x0}, {0x0, 0x0, 0x0}, {0x0, ...}, ...})
        /Users/danimar.ribeiro/go/pkg/mod/github.com/!ad!roll/[email protected]/baker_cli.go:35 +0xec
main.main()
        /Users/danimar.ribeiro/Projetos/extra/baker-test/testing.go:17 +0xb4
exit status 2

✅ Checklists

  • Have the changes in this PR been functionally tested?
  • Has make gofmt-write been run on the code?
  • Has make govet been run on the code? Has the code been fixed accordingly to the output?
  • Have the changes been added to the CHANGELOG.md file?

@codecov-commenter
Copy link

Codecov Report

Merging #213 (d5df4c5) into main (40a57bc) will increase coverage by 7.32%.
The diff coverage is 76.63%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #213      +/-   ##
==========================================
+ Coverage   53.95%   61.28%   +7.32%     
==========================================
  Files          60       67       +7     
  Lines        4003     4972     +969     
==========================================
+ Hits         2160     3047     +887     
- Misses       1646     1687      +41     
- Partials      197      238      +41     
Impacted Files Coverage Δ
api.go 100.00% <ø> (ø)
baker.go 0.00% <0.00%> (ø)
baker_cli.go 0.00% <0.00%> (ø)
filter/clausefilter.go 73.50% <0.00%> (+1.18%) ⬆️
filter/clear_fields.go 75.00% <0.00%> (+10.71%) ⬆️
filter/dedup.go 90.32% <0.00%> (+1.86%) ⬆️
filter/expand_json.go 96.61% <0.00%> (+0.45%) ⬆️
filter/format_time.go 97.91% <0.00%> (+0.11%) ⬆️
filter/hash.go 77.55% <0.00%> (+0.46%) ⬆️
filter/metadata_lastmodified.go 89.47% <0.00%> (+0.58%) ⬆️
... and 103 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d34ab0...d5df4c5. Read the comment docs.

Copy link
Collaborator

@arl arl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@arl arl merged commit a3d78e4 into AdRoll:main Dec 12, 2022
@danimaribeiro danimaribeiro deleted the dd-trace-compatibility branch December 12, 2022 17:49
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