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 Pod NodeSelector changes from OpenTelemetryCollector #2941

Merged
merged 6 commits into from
May 10, 2024

Conversation

janario
Copy link
Contributor

@janario janario commented May 9, 2024

Description:

Link to tracking Issue(s):

Testing:

With e2e, create and update collectors (Deployment/Daemonset/Statefulset) and add/remove nodeSelector.

Documentation:

Signed-off-by: Janario Oliveira <[email protected]>
@janario janario requested a review from a team May 9, 2024 11:37
janario added 3 commits May 9, 2024 13:38
Signed-off-by: Janario Oliveira <[email protected]>
Signed-off-by: Janario Oliveira <[email protected]>
@swiatekm
Copy link
Contributor

swiatekm commented May 9, 2024

A reconciliation test for OpampBridge is failing, I'm guessing because it expects an annotation to exist that your change removes. @jaronoff97 might be able to narrow it down more.

@janario
Copy link
Contributor Author

janario commented May 9, 2024

A reconciliation test for OpampBridge is failing, I'm guessing because it expects an annotation to exist that your change removes. @jaronoff97 might be able to narrow it down more.

yes, previously I was trying to merge-override-empty with the entire PodTemplate.
But a few things like labels and annotations are expected to stay even when not managed by the operator.

In the last commit, I changed to merge-override-empty only to the nodeSelector and we add more things as we find later.

@jaronoff97 jaronoff97 requested a review from swiatekm May 9, 2024 15:05
Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

This looks good to me, but we really should figure out a more holistic way of dealing with this problem.

@jaronoff97 jaronoff97 merged commit 904b7ec into open-telemetry:main May 10, 2024
33 checks passed
@jaronoff97
Copy link
Contributor

@swiatekm-sumo agreed, though I think that's something we can consider in a follow up.

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