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

Rename service.Application to service.Collector #3268

Merged

Conversation

eddyleelin
Copy link
Contributor

@eddyleelin eddyleelin commented May 24, 2021

Description:
Breaking change: Changed the naming of the collector server to service.Collector (previously named service.Application, which could be confused with the user applications to instrument).

Link to tracking Issue:
Addresses #3175

Testing:
Local make does not report any errors in the files changed.

Documentation:

  • Add number of upstream PR.

TODO

  • Make changes to contrib repo: PR 3699

@eddyleelin eddyleelin requested a review from a team May 24, 2021 14:20
@carlosalberto
Copy link
Contributor

Small nit: maybe coll as the common var name is better than col, but that's just me (then again, I'm not quite familiar with the go idioms yet ;) )

Copy link
Contributor

@rakyll rakyll left a comment

Choose a reason for hiding this comment

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

One suggestion, otherwise LGTM.

service/settings.go Outdated Show resolved Hide resolved
eddyleelin added a commit to open-o11y/opentelemetry-collector-contrib that referenced this pull request May 27, 2021
Copy link
Contributor

@rakyll rakyll left a comment

Choose a reason for hiding this comment

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

Left some comments.

.github/workflows/snyk-container-analysis.yml Outdated Show resolved Hide resolved
@@ -79,36 +79,36 @@ func (s *WindowsService) Execute(args []string, requests <-chan svc.ChangeReques
return false, 0
}

func (s *WindowsService) start(elog *eventlog.Log, appErrorChannel chan error) error {
func (s *WindowsService) start(elog *eventlog.Log, colErrorChannel chan error) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to rename the WindowsService to WindowsCollector.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, ignore this for now and we can take another loop if any naming is required before we cut the stable release.

@tigrannajaryan
Copy link
Member

I think there is a commit from @xukaren who did not sign the CLA that's why the check is failing. It is best to avoid having multiple people committing to the same PR to avoid confusion.

@rakyll
Copy link
Contributor

rakyll commented Jun 14, 2021

The commits coming from xukaren is not relevant to this PR. Can we fix the PR? This is a big breaking change and we need to merge it, sooner the better.

@xukaren
Copy link
Contributor

xukaren commented Jun 14, 2021

I think there is a commit from @xukaren who did not sign the CLA that's why the check is failing. It is best to avoid having multiple people committing to the same PR to avoid confusion.

I think my commits were included by mistake, and I don't see them in the files changed for this PR.
I've signed the CLA anyway and hopefully this PR is unblocked now.

@alolita
Copy link
Member

alolita commented Jun 14, 2021

@rakyll will get this PR fixed. It might be easier to open a new PR.

@eddyleelin @xukaren please sync up and fix this issue.

@eddyleelin eddyleelin closed this Jun 14, 2021
eddyleelin and others added 9 commits June 14, 2021 18:10
- Changed references in function/method docs from 'application' to 'collector server'
- Changed instance variable names from (app *Application) to (col *Collector)
- renamed files in service/application* to service/collector*
- replaced occurrences of app to col
From v0.28.0-beta to unreleased (breaking change)
@eddyleelin eddyleelin reopened this Jun 14, 2021
@eddyleelin
Copy link
Contributor Author

eddyleelin commented Jun 14, 2021

I apologize for including the incorrect commits by mistake (again, my fault!).

I have rebased all commits that had previously got approved; they now show up with the new commits 5d1987d..ad55466, so the history for the PR should now be clean (even though the previous commits will still show up on this page). It also includes the most recent commit e4fb185 to fix up a small issue where I had deleted a new line on accident during a merge conflict. It seems like the currently failing tests are unrelated to my changes (besides the contrib test, which is addressed by the PR linked in the main comment).

Thanks again to everyone for the patience and help along the way, and sorry again for the trouble! 😄

@alolita
Copy link
Member

alolita commented Jun 15, 2021

@eddyleelin thanks for fixing the commit history. Can you make sure the required tests are passing?

@bogdandrutu
Copy link
Member

@eddyleelin please rebase to fix some of the errors fixed on head

@eddyleelin
Copy link
Contributor Author

Awesome, thanks @bogdandrutu !

@hankveal181
Copy link

Metropolitan Commercial Bank

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

Successfully merging this pull request may close these issues.

9 participants