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.

Sign up for GitHub

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

Jump to bottom

Redefine thrift-gen types as aliases to jaeger-idl #6617

Closed
5 of 6 tasks
yurishkuro opened this issue Jan 27, 2025 · 4 comments
Closed
5 of 6 tasks

Redefine thrift-gen types as aliases to jaeger-idl #6617

yurishkuro opened this issue Jan 27, 2025 · 4 comments
Labels
enhancement good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@yurishkuro
Copy link
Member

yurishkuro commented Jan 27, 2025 •
edited
Loading

The jaeger-idl now has a copy of thrift-gen types. We want to remove jaeger's dependency on jaeger/thrift-gen in favor of jaeger-idl/thrift-gen,
@yurishkuro yurishkuro added good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement labels Jan 27, 2025
@dosubot dosubot bot added the enhancement label Jan 27, 2025
@danish9039
Copy link
Contributor

working on it

This was referenced Jan 27, 2025
Merged
Merged
yurishkuro pushed a commit that referenced this issue Jan 27, 2025
@danish9039
Redefine thrift-gen types as aliases to jaeger-idl (#6619)
20bdd8d 
## Which problem is this PR solving?
- #6617

## Description of the changes
- remove auto-generated code and replace its types / functions with
aliases to jaeger-idl hosted types

## How was this change tested?
- 

## Checklist
- [ ] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [ ] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [ ] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `npm run lint` and `npm run test`

---------

Signed-off-by: danish9039 <[email protected]>
@yurishkuro
Copy link
Member Author

added a few more items, we can clean up Thrift completely

@Nabil-Salah Nabil-Salah mentioned this issue Jan 29, 2025
Merged
4 tasks
yurishkuro pushed a commit to jaegertracing/jaeger-idl that referenced this issue Jan 29, 2025
@Nabil-Salah
Migrate sampling.thrift (#132)
5fa35ae 
## Which problem is this PR solving?
- jaegertracing/jaeger#6617

## Description of the changes
- generate sampling.thrift code

## How was this change tested?
- ci

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [ ] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `npm run lint` and `npm run test`

Signed-off-by: nabil salah <[email protected]>
@Nabil-Salah Nabil-Salah mentioned this issue Jan 29, 2025
Merged
4 tasks
yurishkuro pushed a commit that referenced this issue Jan 29, 2025
@Nabil-Salah
Alias samping.thrift and clean thrift files (#6630)
10bacb7 
## Which problem is this PR solving?
- #6617

## 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
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `npm run lint` and `npm run test`

---------

Signed-off-by: nabil salah <[email protected]>
Signed-off-by: Nabil Salah <[email protected]>
@yurishkuro
Copy link
Member Author

@Nabil-Salah we now need to update these imports to point to jaeger-idl:

$ rg jaegertracing/jaeger/thrift-gen
model/converter/json/sampling_test.go
15:	api_v1 "github.com/jaegertracing/jaeger/thrift-gen/sampling"

model/converter/thrift/jaeger/sampling_to_domain.go
10:	"github.com/jaegertracing/jaeger/thrift-gen/sampling"

model/converter/thrift/jaeger/sampling_from_domain_test.go
14:	"github.com/jaegertracing/jaeger/thrift-gen/sampling"

model/converter/thrift/jaeger/sampling_from_domain.go
11:	"github.com/jaegertracing/jaeger/thrift-gen/sampling"

model/converter/thrift/jaeger/sampling_to_domain_test.go
14:	"github.com/jaegertracing/jaeger/thrift-gen/sampling"

@Nabil-Salah
Copy link
Contributor

okay will do it now

@Nabil-Salah Nabil-Salah mentioned this issue Jan 30, 2025
Merged
4 tasks
yurishkuro pushed a commit that referenced this issue Jan 30, 2025
@Nabil-Salah
Update import paths for jaeger thrift files to use jaeger-idl (#6635)
6d8c70e 
## Which problem is this PR solving?
-
#6617 (comment)

## Description of the changes
- replace use of jaeger-idl/thrift-gen/sampling imports

## How was this change tested?
- `make test lint'

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `npm run lint` and `npm run test`

Signed-off-by: nabil salah <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@yurishkuro @Nabil-Salah @danish9039