Skip to content

PR from patch-prchecks to main for commit f4111f9#129

Open
rsharath wants to merge 2 commits intomainfrom
patch-prchecks
Open

PR from patch-prchecks to main for commit f4111f9#129
rsharath wants to merge 2 commits intomainfrom
patch-prchecks

Conversation

@rsharath
Copy link
Copy Markdown
Contributor

This PR is auto-generated by DevOps PR Approval Pipeline

@rsharath rsharath requested a review from akhiljavelin April 21, 2026 03:18
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request standardizes ingress configurations across AKS and EKS by updating hostnames and prefixing service paths with /v1. It also synchronizes liveness and readiness probes with these new paths and adjusts ALB group ordering for EKS. Feedback identifies inconsistencies between container readiness probes and ingress health checks for the collector service, and points out a recurring typo (heathz instead of healthz) in the firehog service's health probe paths.

readinessProbe:
httpGet:
path: /
path: /v1/otel-http
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The readiness probe path is being changed to /v1/otel-http on port 13133. This is inconsistent with the ingress health probe path /v1/otel-http/healthz defined on line 79. If the application serves health status at /v1/otel-http/healthz, this probe should likely match to ensure consistency between container and ingress health checks.

readinessProbe:
httpGet:
path: /
path: /v1/otel-http
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The readiness probe path is being changed to /v1/otel-http on port 13133. This is inconsistent with the ingress health check path /v1/otel-http/healthz defined on line 76.

Comment thread helm-values/AKS/highflame-firehog-helm-values-tmpl.yml Outdated
Comment thread helm-values/AKS/highflame-firehog-helm-values-tmpl.yml Outdated
Comment thread helm-values/AKS/highflame-firehog-helm-values-tmpl.yml Outdated
Comment thread helm-values/EKS/highflame-firehog-helm-values-tmpl.yml Outdated
Comment thread helm-values/EKS/highflame-firehog-helm-values-tmpl.yml Outdated
Comment thread helm-values/EKS/highflame-firehog-helm-values-tmpl.yml Outdated
@darshana-v
Copy link
Copy Markdown

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.

3 participants