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

Update Go version to 1.20 #4206

Merged
merged 6 commits into from
Mar 4, 2023
Merged

Update Go version to 1.20 #4206

merged 6 commits into from
Mar 4, 2023

Conversation

SaarthakMaini
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

All the files mentioned in issue #4202 have been updated with Go version 1.20 replacing version 1.19

Signed-off-by: Saarthak Maini [email protected]

@SaarthakMaini SaarthakMaini requested a review from a team as a code owner February 4, 2023 03:24
yurishkuro
yurishkuro previously approved these changes Feb 4, 2023
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thanks! Please make sure that all commits (or squash into one) are signed per CONTRIBUTING.md

@yurishkuro yurishkuro changed the title Updated Go Version from 1.19 to 1.20 Update Go version to 1.20 Feb 4, 2023
@SaarthakMaini
Updated Go Version from 1.19 to 1.20
8e48e04 
All the files mentioned in issue jaegertracing#4202 have been updated with Go version
1.20 replacing version 1.19

Signed-off-by: Saarthak <[email protected]>
@SaarthakMaini
Copy link
Contributor Author

I have signed the DCO as required!

Thank You Very Much for reviewing the changes

@albertteoh
Copy link
Contributor

I think the unit test failures relate to golangci-lint incompatibility with go 1.20. We can try upgrading to golangci-lint v1.51.0 in the Makefile here.

@SaarthakMaini
Update golangci-lint version to v1.51.0
4c485d2 
Updates the Makefile to make golangci-lint compatible with Go v1.20

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

Linter failing

Error: cmd/anonymizer/app/query/query.go:83:3: S1011: should replace loop with spans = append(spans, received.Spans...) (gosimple)

@SaarthakMaini
Solve S1011: Linter Error
4756c24 
Use a single \'append\' to concatenate two slices

ie No for loop required

Signed-off-by: Saarthak <[email protected]>
@SaarthakMaini
Copy link
Contributor Author

Linter was failing due to check S1011

I have made the necessary changes

yurishkuro
yurishkuro previously approved these changes Feb 5, 2023
View reviewed changes
@SaarthakMaini
Copy link
Contributor Author

There was a merge conflict which was happening, so I pulled the latest changes from the main repo to my fork, then git pull (ed) the changes to the branch I'm working in locally, and then after merging the changes I pushed it again to Github

The previous update with merge conflict was giving Unit Test error and Kafka error

@SaarthakMaini
Copy link
Contributor Author

The Unit Tests seem to be failing with exit code 2

What I have speculated is that:

  • The problem might be in running and getting golangci-lint as the error is occurring at line 162 of Makefile
  • We have to replace the make command with the make all command. Reference
  • The error occurred is the following:
    make: *** [Makefile:162: lint] Error 1
    Error: Process completed with exit code 2.
  • I can also try undoing the version update I did to update golangci-lint to v1.51.0

Please let me know some suggestions and problems which I might be making for the same

Thank You!

@yurishkuro
Copy link
Member

I see this error in the output

cmd/anonymizer/app/query/query.go:83: File is not `gofmt`-ed with `-s` (gofmt)
		spans = append(spans, received.Spans...)

@yurishkuro
Copy link
Member

try running make fmt

@SaarthakMaini
Copy link
Contributor Author

I'm getting the following error on running make fmt in the jaegar folder:
make fmt
./scripts/import-order-cleanup.sh inplace
bash: C:\Users\saarthak: No such file or directory
make: *** [Makefile:153: fmt] Error 127

I came across this article and am currently trying to solve it this way

Thank you for reviewing the changes and helping me along the way!

@codecov
Copy link

codecov bot commented Feb 5, 2023 •
edited
Loading

Codecov Report

Patch coverage has no change and project coverage change: -0.03 ⚠️

Comparison is base (82d7f9b) 97.09% compared to head (e929913) 97.07%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4206      +/-   ##
==========================================
- Coverage   97.09%   97.07%   -0.03%     
==========================================
  Files         302      302              
  Lines       17685    17685              
==========================================
- Hits        17172    17168       -4     
- Misses        413      416       +3     
- Partials      100      101       +1
Flag Coverage Δ
unittests 97.07% <ø> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
pkg/config/tlscfg/cert_watcher.go 92.59% <0.00%> (-2.23%) ⬇️
...lugin/sampling/strategystore/adaptive/processor.go 99.38% <0.00%> (-0.62%) ⬇️
plugin/storage/integration/integration.go 76.33% <0.00%> (+0.38%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yurishkuro
Copy link
Member

Thanks. I'm not going to merge it just yet, but for the next release.

@yurishkuro yurishkuro enabled auto-merge (squash) March 4, 2023 02:55
@yurishkuro yurishkuro merged commit 1d3bc1d into jaegertracing:main Mar 4, 2023
shubbham1215 pushed a commit to shubbham1215/jaeger that referenced this pull request Mar 5, 2023
@SaarthakMaini @yurishkuro @shubbham1215
Update Go version to 1.20 (jaegertracing#4206)
ec3d83e 
## Which problem is this PR solving?
- Resolves jaegertracing#4202

## Short description of the changes
All the files mentioned in issue jaegertracing#4202 have been updated with Go version
1.20 replacing version 1.19

Signed-off-by: Saarthak Maini <[email protected]>

---------

Signed-off-by: Saarthak <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
Signed-off-by: shubbham1215 <[email protected]>
@mx-psi mx-psi mentioned this pull request Mar 22, 2023
Closed
mx-psi added a commit to mx-psi/jaeger that referenced this pull request Mar 22, 2023
@mx-psi
Change go.mod's go directive to indicate support for Go 1.19
8de19fa 
This partially reverts jaegertracing#4206.

On the OpenTelemetry Collector project we aim to provide support
for all officially supported Go versions. Per Go's support policy [1],
Go 1.19 will remain a supported Go version until Go 1.21 is released.
Since we take a dependency on Jaeger as a library, we need Jaeger to
remain compatible with Go 1.19 as a library to be able to keep it updated.

This patch does not revert other changes on jaegertracing#4206; Jaeger binaries would
continue to be built with Go 1.20 and benefit from all performance and security
improvements that the 1.20 release cycle offers.

Signed-off-by: Pablo Baeyens <[email protected]>
@mx-psi mx-psi mentioned this pull request Mar 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@joe-elliott joe-elliott Awaiting requested review from joe-elliott joe-elliott is a code owner automatically assigned from jaegertracing/jaeger-maintainers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature]: Migrate to go 1.20
3 participants
@SaarthakMaini @albertteoh @yurishkuro