-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Separate /model into its own module #5144
Conversation
Signed-off-by: Chirag Ghosh <[email protected]>
Signed-off-by: Chirag Ghosh <[email protected]>
Signed-off-by: Chirag Ghosh <[email protected]>
Signed-off-by: Chirag Ghosh <[email protected]>
require ( | ||
github.com/apache/thrift v0.19.0 | ||
github.com/gogo/protobuf v1.3.2 | ||
github.com/jaegertracing/jaeger v0.0.0-00010101000000-000000000000 |
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.
Does this mean we have circular dependency? Because in that case the whole exercise is pointless, the whole reason for separating model was so that OTEL contrib could import it with the rest of jaeger deps. I think model should have very minimal deps, we can probably separate them.
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 we can remove this. this was automatically added when i ran go mod tidy
.
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.
The only dependency of jaeger in /model
is in tests like here. I think we need to have the testutils inside /model
as well to remove the dependency.
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 see model/adjuster/package_test.go
Pointing to pkg/utils, which can explain the circular dependency.
There might be more that this one
Edit. There is also an import from thrift-gen and proto-gen packages.
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.
to remove the circular dependency we will need to either copy the same things inside /model or make them separate packages as well
@@ -22,6 +22,7 @@ require ( | |||
github.com/grpc-ecosystem/go-grpc-middleware v1.4.0 | |||
github.com/hashicorp/go-hclog v1.6.2 | |||
github.com/hashicorp/go-plugin v1.6.0 | |||
github.com/jaegertracing/jaeger/model v0.0.0-00010101000000-000000000000 |
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.
What is the significance of this weird version? Is dependabot going to try to change it? Did you try on a separate repo?
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 will try in my repository and see what happens
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5144 +/- ##
==========================================
- Coverage 95.62% 95.34% -0.28%
==========================================
Files 324 324
Lines 18590 18590
==========================================
- Hits 17776 17725 -51
- Misses 653 707 +54
+ Partials 161 158 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@yurishkuro Can you please help me figure out why the code coverage went down? |
In the report I see that all /model/ files are down to 0% coverage. I suspect |
another issue - the vulnerability checker flagged /model/ as vulnerable dependency, I suspect because of 0.x version number. What is the story with this version number? In OTEL collector I see that their versions are in sync across modules, so they must have some mechanism for managing/updating those after release.
|
Signed-off-by: Chirag Ghosh <[email protected]>
The wierd versioning happended after I aded the replace lines and ran |
Signed-off-by: Chirag Ghosh <[email protected]>
closing #4899 (comment) |
Which problem is this PR solving?
Description of the changes
/model
. Used replace in go.mod at/
and/model
/scripts/check-go-version.sh
to check/model/go.mod
as well.How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test