Adds vanity import check by jcchavezs · Pull Request #4180 · open-telemetry/opentelemetry-collector

jcchavezs · 2021-10-06T20:25:07Z

Important

This PR fixes the vanity imports and add a check to verify them on CI.

Follows from open-telemetry/opentelemetry-go#2255

codecov · 2021-10-06T20:51:47Z

Codecov Report

Merging #4180 (84ec462) into main (6654804) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #4180   +/-   ##
=======================================
  Coverage   87.73%   87.73%           
=======================================
  Files         174      174           
  Lines       10335    10335           
=======================================
  Hits         9067     9067           
  Misses       1016     1016           
  Partials      252      252

Impacted Files	Coverage Δ
client/client.go	`85.71% <ø> (ø)`
component/build_info.go	`100.00% <ø> (ø)`
component/componenthelper/component.go	`100.00% <ø> (ø)`
component/componenttest/nop_exporter.go	`100.00% <ø> (ø)`
component/componenttest/nop_extension.go	`100.00% <ø> (ø)`
component/componenttest/nop_factories.go	`29.41% <ø> (ø)`
component/componenttest/nop_host.go	`100.00% <ø> (ø)`
component/componenttest/nop_processor.go	`100.00% <ø> (ø)`
component/componenttest/nop_receiver.go	`100.00% <ø> (ø)`
component/componenttest/nop_telemetry.go	`100.00% <ø> (ø)`
... and 158 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6654804...84ec462. Read the comment docs.

mx-psi

After this PR the Collector will have the same issue (open-telemetry/opentelemetry-go#2279) that the Go instrumentation library has today: internal packages are not checked so they can end up having an incorrect vanity import comment (which breaks some builds).

That can be okay in the short term but we should think if we want to either (i) disallow vanity import comments on internal packages or (ii) add them too and check them with porto.

jcchavezs · 2021-10-07T12:12:23Z

Ping @bogdandrutu

…

On Thu, Oct 7, 2021, 12:15 Pablo Baeyens ***@***.***> wrote: ***@***.**** commented on this pull request. After this PR the Collector will have the same issue ( open-telemetry/opentelemetry-go#2279 <open-telemetry/opentelemetry-go#2279>) that the Go instrumentation library has today: internal packages are not checked so they can end up having an incorrect vanity import comment (which breaks some builds). That can be okay in the short term but we should think if we want to either (i) disallow vanity import comments on internal packages or (ii) add them too and check them with porto. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4180 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXOYAVZKQJZKJNJ5H3AVPTUFVXK3ANCNFSM5FPT5CUQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

bogdandrutu · 2021-10-11T01:56:29Z

@jcchavezs what is the solution/proposal for @mx-psi issue?

jcchavezs · 2021-10-12T11:59:15Z

@bogdandrutu I think the codebase is big enough to be able to use vanity import for internal packages. @mx-psi already introduced a great addition to include internal packages in this PR so I guess it is only a matter to agree to include internal packages here and opentelemetry-go (following open-telemetry/opentelemetry-go#2277). Does that make sense @bogdandrutu @mx-psi?

bogdandrutu · 2021-10-12T16:30:56Z


+.PHONY: porto
+porto:
+	porto -w ./


Should we use "--include-internal" ?

mx-psi · 2021-10-12T16:34:05Z

I think the same solution as in opentelemetry-go makes sense, it looks like for this repo we probably just need a new porto release with jcchavezs/porto#11 and jcchavezs/porto#12 for that to happen :)

jcchavezs · 2021-10-12T19:16:43Z

Done @mx-psi https://github.com/jcchavezs/porto/releases/tag/v0.3.0

jcchavezs · 2021-10-12T19:16:57Z

I will update this PR accordingly.

bogdandrutu · 2021-10-12T19:22:35Z

@jcchavezs the lint job fails.

codeboten

Can't find porto for the lint job

porto -w ./
/bin/bash: porto: command not found

jcchavezs · 2021-10-12T20:45:56Z

@bogdandrutu failure seems unrelated. Could you please retrigger?

jcchavezs requested review from a team and jpkrohling October 6, 2021 20:25

jcchavezs changed the title ~~Adds go porto~~ Adds vanity import check Oct 6, 2021

bogdandrutu reviewed Oct 6, 2021

View reviewed changes

Comment thread .github/workflows/build-and-test.yml

jcchavezs commented Oct 6, 2021

View reviewed changes

Comment thread .github/workflows/build-and-test.yml

jcchavezs commented Oct 6, 2021

View reviewed changes

Comment thread .github/workflows/build-and-test.yml

mx-psi reviewed Oct 7, 2021

View reviewed changes

bogdandrutu approved these changes Oct 11, 2021

View reviewed changes

bogdandrutu reviewed Oct 12, 2021

View reviewed changes

Comment thread Makefile.Common Outdated

.PHONY: porto

porto:

porto -w ./

Copy link
Copy Markdown

Member

bogdandrutu Oct 12, 2021

Choose a reason for hiding this comment

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

Should we use "--include-internal" ?

codeboten reviewed Oct 12, 2021

View reviewed changes

jcchavezs force-pushed the adds_go_porto branch from 4b5571c to a8af3a4 Compare October 12, 2021 19:55

jcchavezs added 7 commits October 12, 2021 22:20

chore: adds porto and fixes vanity imports.

f246529

chore: fixes target overriding.

b891777

chore: fixes install of porto.

8366ea8

chore: includes porto as a tool.

0fa6bbc

chore: upgrades porto to check internals.

8e2c838

chore: rebase and update vanity import.

218dbfc

chore: removes unnecessary space.

1f87a62

jcchavezs force-pushed the adds_go_porto branch from a8af3a4 to 1f87a62 Compare October 12, 2021 20:23

chore: rollsback vanity import in generated files.

84ec462

bogdandrutu merged commit 9d3a8a4 into open-telemetry:main Oct 12, 2021

jcchavezs deleted the adds_go_porto branch October 12, 2021 20:52

jcchavezs mentioned this pull request Nov 15, 2021

chore: adds vanity import check. open-telemetry/opentelemetry-collector-contrib#6305

Merged

jcchavezs mentioned this pull request Sep 1, 2023

chore: adds vanity import to code. istio/istio#46810

Closed

13 tasks

Conversation

jcchavezs commented Oct 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Important

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Oct 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mx-psi left a comment

Choose a reason for hiding this comment

Uh oh!

jcchavezs commented Oct 7, 2021 via email

Uh oh!

bogdandrutu commented Oct 11, 2021

Uh oh!

jcchavezs commented Oct 12, 2021

Uh oh!

bogdandrutu Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

mx-psi commented Oct 12, 2021

Uh oh!

jcchavezs commented Oct 12, 2021

Uh oh!

jcchavezs commented Oct 12, 2021

Uh oh!

bogdandrutu commented Oct 12, 2021

Uh oh!

codeboten left a comment

Choose a reason for hiding this comment

Uh oh!

jcchavezs commented Oct 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jcchavezs commented Oct 6, 2021 •

edited

Loading

codecov Bot commented Oct 6, 2021 •

edited

Loading