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

Make the clusterName value optional in EKS and GKE #1056

Closed
wants to merge 7 commits into from

Conversation

crobert-1
Copy link
Contributor

@crobert-1 crobert-1 commented Dec 1, 2023

Description:

Now that the resource detection processor supports detecting the k8s.cluster.name in GKE and EKS environments, the clusterName value can be optional in these environments.

A simplified resource detection processor has been added to more pipelines in this change as well, in every place the clusterName was manually being added.

Testing:
Tested in GKE and kOps. GKE sets cluster name properly without it specified, and overrides the detected k8s.cluster.name when clusterName is set. kOps fails without clusterName with the following message:

crobert$ ~/dev/splunk-otel-collector-chart $ helm upgrade ...
Error: UPGRADE FAILED: values don't meet the specifications of the schema(s) in the following chart(s):
splunk-otel-collector:
- (root): Must validate "then" as "if" was valid
- clusterName: String length must be greater than or equal to 1
- (root): Must validate all the schemas (allOf)

The - (root) lines are a bit confusing, but that's simply saying that the check failed, AFAICT.

Documentation:
Updated README and relevant comments in code.

@crobert-1 crobert-1 requested review from a team as code owners December 1, 2023 23:13
@jvoravong
Copy link
Contributor

To get the EKS functional-tests up and running, you will have to close this PR and open up a different PR in a specific way due to recent project test updates. The EKS functional tests need to run with credentials that are only available in this repo.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@jvoravong
Copy link
Contributor

These changes LGTM. A couple small nits. Will approve a separate PR that has the EKS functional tests passing.

@atoulme
Copy link
Contributor

atoulme commented Dec 4, 2023

Indeed, please push to this repository and open a PR rather than from your fork.

crobert-1 and others added 2 commits December 7, 2023 13:05
Co-authored-by: jvoravong <47871238+jvoravong@users.noreply.github.com>
Co-authored-by: jvoravong <47871238+jvoravong@users.noreply.github.com>
@crobert-1
Copy link
Contributor Author

Closing in favor of #1067

@crobert-1 crobert-1 closed this Dec 8, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants