Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Alias samping.thrift and clean thrift files #6630

Merged
merged 10 commits into from
Jan 29, 2025

Conversation

Nabil-Salah
Copy link
Contributor

@Nabil-Salah Nabil-Salah commented Jan 29, 2025

Which problem is this PR solving?

Description of the changes

  • remove unused Thrift files
  • remove thrift makefile
  • edit makefile
  • alias samping.thrift

How was this change tested?

  • make test lint

Checklist

Signed-off-by: nabil salah <nabil.salah203@gmail.com>
@Nabil-Salah Nabil-Salah requested a review from a team as a code owner January 29, 2025 15:39
Signed-off-by: nabil salah <nabil.salah203@gmail.com>
@Nabil-Salah
Copy link
Contributor Author

I am not seeing any aliases added

I made alias in thrift-gen/sampling/sampling.go and removed baggage.thrift generations

@@ -3,1361 +3,57 @@
package sampling
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: instead of editing generated file, delete it and create another one sampling_alias.go. Don't forget to run make fmt to add the license header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay will make these updates in a second

Copy link
Contributor Author

@Nabil-Salah Nabil-Salah Jan 29, 2025

Choose a reason for hiding this comment

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

suggestion: instead of editing generated file, delete it and create another one sampling_alias.go. Don't forget to run make fmt to add the license header.

@yurishkuro
changing the name didn't update
I updated makefile take a look for that I think now as we are migrating all model types and making alias only it's okay to make this change to add these files to non generated files

Copy link
Member

@yurishkuro yurishkuro Jan 29, 2025

Choose a reason for hiding this comment

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

changing the name didn't update

what do you mean? mv thrift-gen/sampling/sampling.go thrift-gen/sampling/sampling_aliases.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes when i do that it willn't get effected by make fmt

.PHONY: fmt
fmt: $(GOFUMPT)
	@echo Running import-order-cleanup on ALL_SRC ...

fmt uses ALL_SRC variable that exclude -gen folders I updated this in makefile

Copy link

codecov bot commented Jan 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.93%. Comparing base (97e800d) to head (7a03949).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6630      +/-   ##
==========================================
- Coverage   95.96%   95.93%   -0.03%     
==========================================
  Files         365      365              
  Lines       20616    20616              
==========================================
- Hits        19784    19778       -6     
- Misses        634      638       +4     
- Partials      198      200       +2     
Flag Coverage Δ
badger_v1 9.92% <ø> (ø)
badger_v2 1.84% <ø> (ø)
cassandra-4.x-v1-manual 14.93% <ø> (ø)
cassandra-4.x-v2-auto 1.83% <ø> (ø)
cassandra-4.x-v2-manual 1.83% <ø> (ø)
cassandra-5.x-v1-manual 14.93% <ø> (ø)
cassandra-5.x-v2-auto 1.83% <ø> (ø)
cassandra-5.x-v2-manual 1.83% <ø> (ø)
elasticsearch-6.x-v1 19.30% <ø> (ø)
elasticsearch-7.x-v1 19.38% <ø> (ø)
elasticsearch-8.x-v1 19.56% <ø> (ø)
elasticsearch-8.x-v2 1.84% <ø> (ø)
grpc_v1 10.90% <ø> (ø)
grpc_v2 7.89% <ø> (ø)
kafka-3.x-v1 10.21% <ø> (ø)
kafka-3.x-v2 1.84% <ø> (ø)
memory_v2 1.84% <ø> (ø)
opensearch-1.x-v1 19.44% <ø> (ø)
opensearch-2.x-v1 19.44% <ø> (ø)
opensearch-2.x-v2 1.84% <ø> (ø)
tailsampling-processor 0.48% <ø> (ø)
unittests 94.81% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…r to be fmted

Signed-off-by: nabil salah <nabil.salah203@gmail.com>
Signed-off-by: nabil salah <nabil.salah203@gmail.com>
Signed-off-by: Nabil Salah <nabil.salah203@gmail.com>
@Nabil-Salah Nabil-Salah changed the title Alias samping.thrift and clean thrift files Alias samping.thrift and clean thrift files and alter imports to jaeger-idl Jan 29, 2025
Signed-off-by: nabil salah <nabil.salah203@gmail.com>
Signed-off-by: nabil salah <nabil.salah203@gmail.com>
Signed-off-by: nabil salah <nabil.salah203@gmail.com>
Signed-off-by: nabil salah <nabil.salah203@gmail.com>
@Nabil-Salah Nabil-Salah changed the title Alias samping.thrift and clean thrift files and alter imports to jaeger-idl Alias samping.thrift and clean thrift files Jan 29, 2025
@yurishkuro yurishkuro enabled auto-merge (squash) January 29, 2025 19:01
@yurishkuro yurishkuro merged commit 10bacb7 into jaegertracing:main Jan 29, 2025
55 checks passed
@Nabil-Salah Nabil-Salah deleted the remove_thrift branch January 29, 2025 19:07
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants