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 service account is created even when one is specified in collector spec #2378

Merged
merged 6 commits into from
Nov 24, 2023

Conversation

changexd
Copy link
Contributor

Description:
Fix service account is created even when one is specified in collector spec

Link to tracking Issue:
#2372

@changexd changexd requested a review from a team November 21, 2023 03:22
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.

Could you add a test verifying that the ServiceAccount doeesn't get created if the value is set? LGTM otherwise.

@@ -45,12 +45,14 @@ func Build(params manifests.Params) ([]client.Object, error) {
manifestFactories = append(manifestFactories, []manifests.K8sManifestFactory{
manifests.FactoryWithoutError(ConfigMap),
manifests.FactoryWithoutError(HorizontalPodAutoscaler),
manifests.FactoryWithoutError(ServiceAccount),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we instead just check this in the service account manifest? In the builder we would then return nil if service account is set

@changexd
Copy link
Contributor Author

changexd commented Nov 22, 2023

I wasn't sure if the test should be placed in service account manifest, so I simply added test to builder since we would like to check if the builder build service account manifest when specified, feel free to let me know your thoughts : >

@jaronoff97 jaronoff97 requested a review from swiatekm November 24, 2023 14:57
@pavolloffay pavolloffay merged commit 7b9d00b into open-telemetry:main Nov 24, 2023
26 checks passed
@changexd changexd deleted the sa-fix branch December 20, 2023 02:37
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
…r spec (open-telemetry#2378)

* add additional check before creating sa for collector

* update builder test for collector

* change check collector sa specified in sa manifest

* add specified account test case to collector building
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.

4 participants