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

chore: Moves internal/kafka To pkg/kafka #33181

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wicklander-bryant
Copy link

Description:
Fixing a bug - As described in the github comment here (#27289 (comment)), users will encounter an error when attempting to use the kafka exporter for a custom collector which requires authentication of any kind. This is because the Authentication field is referencing a struct that is in an internal package, see here.

Link to tracking Issue: #33180

Testing: Seeing as this isn't as much of a functional changes as it is an organizational change the existing tests will cover the changes.

Documentation: N/A - This is a smaller organizational change that is not accompanied by documentation.

Copy link

linux-foundation-easycla bot commented May 22, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: wicklander-bryant / name: Trece Wicklander-Bryant (2aa7039)

@strawgate
Copy link
Contributor

I'm not an approver but thanks for taking this on and I don't see anything to comment on in the PR

@wicklander-bryant
Copy link
Author

I'm not an approver but thanks for taking this on and I don't see anything to comment on in the PR

No worries at all, thanks for giving it a look through though!

@wicklander-bryant wicklander-bryant force-pushed the mv-kafka-pkg branch 4 times, most recently from c74c952 to cf6aced Compare May 24, 2024 07:00
@wicklander-bryant
Copy link
Author

wicklander-bryant commented May 24, 2024

I noticed that the gen genotelcontribcol check failed and specified the fix is to run make genotelcontrobcol and commit the changes:

/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/.tools/goimports -w  -local github.com/open-telemetry/opentelemetry-collector-contrib ./
Generated code is out of date, please run "make genotelcontribcol" and commit the changes in this PR.

When I run the make genotelcontribcol then run git status there are no changes to commit. The output from running make genotelcontribcol is as follows:

/Users/t.wicklander-bryant/code/personal/opentelemetry-collector-contrib/.tools/builder --skip-compilation --config cmd/otelcontribcol/builder-config.yaml --output-path cmd/otelcontribcol
Flag --output-path has been deprecated, use config distribution::output_path
2024-05-24T00:01:50.152-0700    INFO    internal/command.go:125 OpenTelemetry Collector Builder {"version": "", "date": "unknown"}
2024-05-24T00:01:50.158-0700    INFO    internal/command.go:161 Using config file       {"path": "cmd/otelcontribcol/builder-config.yaml"}
2024-05-24T00:01:50.158-0700    INFO    builder/config.go:132   Using go        {"go-executable": "/usr/local/go/bin/go"}
2024-05-24T00:01:50.166-0700    INFO    builder/main.go:100     Sources created {"path": "cmd/otelcontribcol"}
2024-05-24T00:01:50.671-0700    INFO    builder/main.go:191     Getting go modules
2024-05-24T00:01:50.929-0700    INFO    builder/main.go:107     Generating source codes only, the distribution will not be compiled.
/Library/Developer/CommandLineTools/usr/bin/make --no-print-directory -C cmd/otelcontribcol fmt
Makefile:4: warning: overriding commands for target `lint'
../../Makefile.Common:199: warning: ignoring old commands for target `lint'
gofmt  -w -s ./
/Users/t.wicklander-bryant/code/personal/opentelemetry-collector-contrib/.tools/goimports -w  -local github.com/open-telemetry/opentelemetry-collector-contrib ./

The main branch that this MR targets ended up getting a few new commits and I ended up needing to rebase this branch. Since the target branch was ahead a few commits is that what caused the check failure? It makes sense to me because it's possible that the generated code became out of date. Just want to make sure I don't need to run something else in order to properly generate the code so the check will be happy with me!

@wicklander-bryant wicklander-bryant force-pushed the mv-kafka-pkg branch 5 times, most recently from 6395010 to bcbfcfc Compare May 28, 2024 18:43
@CodeMonkeyF5
Copy link

Following up on this as it's stuck awaiting rebase. @wicklander-bryant would you be able to do that or provide me access to your fork repo?

@wicklander-bryant wicklander-bryant requested a review from a team as a code owner September 18, 2024 21:04
@wicklander-bryant wicklander-bryant force-pushed the mv-kafka-pkg branch 11 times, most recently from 9aa2880 to 0147ae9 Compare September 24, 2024 18:54
@wicklander-bryant
Copy link
Author

Hello @MovieStoreGuy, @dmitryax, and @TylerHelmuth

Could I please get some assistance on getting this PR merged? I've got the necessary approvals but the PR checks are not being started (I can't do this b/c the workflow requires approval from a maintainer). Seeing as you guys are code owners, could one of you please kick off the checks for this PR?

Additionally if the checks do pass will I be able to merge this PR or will one of maintainers need to do this? The reason I ask is b/c right next to the Merge pull request button I see you're not authorized to merge this pull request. I'm not sure if this is b/c the checks haven't passed or if I really can't merge even with successful checks and approvals. My fear is that if the checks pass and this is ready to merge by the time one of you guys goes to merge it there will be conflicts b/c this PR gets conflicts pretty easily due to the nature of the small change.

Any guidance and help is very much appreciated!

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 16, 2024
@andrzej-stencel
Copy link
Member

My fear is that if the checks pass and this is ready to merge by the time one of you guys goes to merge it there will be conflicts b/c this PR gets conflicts pretty easily due to the nature of the small change.

I'm afraid this is the reality we all live in. 😟

Can you please resolve conflicts once again and hopefully one of the maintainers will merge this just in time before new conflicts pop up. 🤞

Copy link
Contributor

github-actions bot commented Nov 1, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Nov 1, 2024
@@ -0,0 +1,1391 @@
// Code generated by "go.opentelemetry.io/collector/cmd/builder". DO NOT EDIT.
Copy link
Contributor

Choose a reason for hiding this comment

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

this file should not be in the source tree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants