-
Notifications
You must be signed in to change notification settings - Fork 435
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
contrib: move some packages into submodules #922
Conversation
github.com/stretchr/testify v1.7.0 | ||
google.golang.org/api v0.46.0 | ||
google.golang.org/grpc v1.37.0 | ||
gopkg.in/DataDog/dd-trace-go.v1 v1.32.0-alpha.13 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have some sort of mechanism that warns us when we release a new version and we forget to bump the integrations. Perhaps a non-blocking CI check (like codecov) would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may not need to keep the integrations' mod files up to date with the latest tracer version. I don't see harm in leaving the minimum compatible version.
I do think we should have the submodule tests replace their copy of dd-trace-go with the "local" copy, so we know when a change in dd-trace-go breaks submodules before we merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may not need to keep the integrations' mod files up to date with the latest tracer version. I don't see harm in leaving the minimum compatible version.
Are you sure? When we fix critical bugs, or add new product features (such as stats computation for example), forgetting to bump the version here will cause this module to not benefit of that feature, resulting in a broken product. To me it seems quite critical to be on the edge.
I do think we should have the submodule tests replace their copy of dd-trace-go with the "local" copy, so we know when a change in dd-trace-go breaks submodules before we merge.
I think the submodule should always be tested against the version in go.mod
, because that's what runs on the user's machine. At the same time, I do agree that any changes made to the trunk of dd-trace-go should have integration tests run against it to ensure all ground is covered.
As we have previously discovered, there is quite a bit of ground to cover here and things to consider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is going to be a nice change as long as it doesn't break anyone.
@gbbr My only concern is that there is a required path change. They won't be able to use the This seems like borderline for breaking changes. We're not breaking API compatibility, but they will have to change the import in their code. Since the burden on users is quite small (probably a single import change) I think we should consider doing it so we can make some progress on this modules issue. |
I'm not sure I understand. In code, the user should have to import the same path for each integration, no? |
@@ -0,0 +1,14 @@ | |||
module github.com/DataDog/dd-trace-go/contrib/cloud.google.com/go/pubsub.v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why github
? gopkg.in
doesn't work? FWIW, if so, I've seen many repositories do this change. It's not that bad IMHO. Worst case scenario we can provide a script which automatically updates their imports 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@knusbaum sorry for the late reply, I really appreciate your work on this! I just had to finish some stuff with deadlines, but now this is done!
Anyway, I think this is going in the right direction, but I share your concern about changing the import path, as well as @gbbr's concerns about each sub module having to potentially bump the version of importing the parent module. But it seems like that'll be easy to automate if we have to.
I general I think this PR feels like a v2 kind of change. IMO changing the import path should be same as an API change that is renaming a method. So if we continue this path we should take a look at other issues we might want to tackle at the same time. Additionally we should decide if we want to turn all contrib modules into sub-modules while we're at it, arguably it would be weird to only do it for a subset of packages.
I'd be open to "pull the trigger" on v2 changes, but my initial hope was to work around the go build ./...
issue in a way that doesn't impact anybody except perhaps grpc v1.2 customers.
What are your thoughts @knusbaum - are you ready for the bigger v2 scope with this? Or do you disagree that the import path change means we need to tackle the other stuff now as well?
With this configuration
go test ./...
, etc. work out of the box for go1.15 and belowDownsides:
go test
, etc. don't work in go1.16+ until the user runsgo mod tidy
.git tag contrib/google.golang.org/grpc/v1.32.0
)I would love to put the true dependencies in go.mod, but it's possible that some of a user's unrelated dependencies will be updated when they add dd-trace-go. This is really undesirable.
Consider the following example:
This is likely to make it difficult for me to adopt dd-trace-go, especially if the go-chi stuff is managed by a different team and/or there are conflicts from upgrading go-chi.
I don't see a way around this besides making every contrib package a module, which we could do in the future.
For now, this helps improve the situation.
Related to #911 #917