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

Fix collector config update #670

Merged
merged 3 commits into from
Jan 27, 2022

Conversation

mcariapas
Copy link
Contributor

I added check sum to pod annotations. This will fix rollout when the collector config changes.
Yes, I know this is duplicated pull request with #506
but the other one has been forgotten for some time.

Fixes #460

@mcariapas mcariapas requested review from a team and Aneurysm9 January 19, 2022 16:59
@linux-foundation-easycla
Copy link

CLA Not Signed

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 19, 2022

CLA Signed

The committers are authorized under a signed CLA.

@mcariapas
Copy link
Contributor Author

@pavolloffay Can you review it?

apis/v1alpha1/zz_generated.deepcopy.go Show resolved Hide resolved
go.sum Outdated Show resolved Hide resolved
Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this and it looks good to me ❤️

I would love to see e2e test for this change (something like this)

  1. deploy collector as deployment
  2. assert it is deployed
  3. change config in CR - e.g. add new receiver
  4. assert the collector pod restarted with the new changes - check if the new receiver port is exposed.

pkg/collector/annotations.go Outdated Show resolved Hide resolved
pkg/collector/annotations.go Show resolved Hide resolved
pkg/collector/annotations.go Outdated Show resolved Hide resolved
go.sum Outdated Show resolved Hide resolved
@linux-foundation-easycla
Copy link

CLA Not Signed

@mcariapas
Copy link
Contributor Author

Thanks for looking into this and it looks good to me ❤️

I would love to see e2e test for this change (something like this)

  1. deploy collector as deployment
  2. assert it is deployed
  3. change config in CR - e.g. add new receiver
  4. assert the collector pod restarted with the new changes - check if the new receiver port is exposed.

Done, please review it.

@linux-foundation-easycla
Copy link

CLA Not Signed

@mcariapas mcariapas force-pushed the pod_sum_annotation branch 2 times, most recently from 557ed9d to f216c31 Compare January 25, 2022 17:14
@linux-foundation-easycla
Copy link

CLA Not Signed

@mcariapas mcariapas force-pushed the pod_sum_annotation branch 2 times, most recently from 1b175bd to f216c31 Compare January 25, 2022 17:20
@pavolloffay pavolloffay changed the title Added opentelemetry-operator-config/sha256 annotation to pod annotations Fix collector config update Jan 26, 2022
@mcariapas
Copy link
Contributor Author

mcariapas commented Jan 27, 2022

@pavolloffay @Aneurysm9 Can we merge it before the next release?

@pavolloffay
Copy link
Member

yes, thanks for the PR :)

cc) @jpkrohling @VineethReddy02 could you please merge?

@jpkrohling jpkrohling merged commit d965ac3 into open-telemetry:main Jan 27, 2022
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
* fix/460 : Added opentelemetry-operator-config/sha256 annotaton to pod annotations

* PR comments fixes

* Created e2e test
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.

Restart collector on config change
4 participants