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.

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

instrgen fixing imports #3682

Merged
merged 3 commits into from
Apr 14, 2023
Merged

Conversation

pdelewski
Copy link
Member

@pdelewski pdelewski commented Apr 5, 2023

Remove import that only is needed in main.go from the other test data files so it will not fail the build.

@pdelewski pdelewski requested review from a team and MrAlias as code owners April 5, 2023 13:14
@pellared
Copy link
Member

pellared commented Apr 5, 2023

How about removing //nolint:all and addressing all the issues? Would it be too much?

@pdelewski
Copy link
Member Author

How about removing //nolint:all and addressing all the issues? Would it be too much?

//nolint:all was added for different reason. Linter is executed at the same time as tests which leads to race conditions and failures

@MrAlias
Copy link
Contributor

MrAlias commented Apr 11, 2023

Why are these imports removed?

@MrAlias
Copy link
Contributor

MrAlias commented Apr 11, 2023

It looks like the tests are failing. Can you take a look at the data race?

@pdelewski
Copy link
Member Author

Why are these imports removed?

This import is needed only in main.go, they are leftovers and if you will try to build this test program compiler will complain about unused imports

@pdelewski
Copy link
Member Author

It looks like the tests are failing. Can you take a look at the data race?

They are unrelated to instrgen

@MrAlias MrAlias added Skip Changelog Allow PR to succeed without requiring an addition to the CHANGELOG area: instrgen Related to the instrgen package labels Apr 11, 2023
@pellared
Copy link
Member

How about removing //nolint:all and addressing all the issues? Would it be too much?

//nolint:all was added for different reason. Linter is executed at the same time as tests which leads to race conditions and failures

Then it would be good to add the reason in a comment e.g. like //nolint:all // Linter is executed at the same time as tests which leads to race conditions and failures. Possibly also track it in some issue to resolve it at some point.

@pdelewski
Copy link
Member Author

How about removing //nolint:all and addressing all the issues? Would it be too much?

//nolint:all was added for different reason. Linter is executed at the same time as tests which leads to race conditions and failures

Then it would be good to add the reason in a comment e.g. like //nolint:all // Linter is executed at the same time as tests which leads to race conditions and failures. Possibly also track it in some issue to resolve it at some point.

Good point.

@MrAlias MrAlias merged commit f887420 into open-telemetry:main Apr 14, 2023
@pellared pellared mentioned this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: instrgen Related to the instrgen package Skip Changelog Allow PR to succeed without requiring an addition to the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants