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

[demo] add imageprovider #1130

Closed
wants to merge 9 commits into from

Conversation

klucsik
Copy link
Contributor

@klucsik klucsik commented Apr 10, 2024

Update demo chart to include open-telemetry/opentelemetry-demo#1462

@klucsik klucsik requested a review from puckpuck as a code owner April 10, 2024 10:26
@klucsik klucsik requested a review from a team April 10, 2024 10:26
@puckpuck
Copy link
Contributor

Because this is adding a new service that has not yet been released, we need to wait for the next demo release before merging this.

Comment on lines 408 to 412
securityContext:
runAsUser: 101 # nginx
runAsGroup: 101
runAsNonRoot: true

Choose a reason for hiding this comment

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

is this working for you? it is triggering /bin/sh: 1: cannot create /etc/nginx/nginx.conf: Permission denied here

- name: IMAGE_PROVIDER_PORT
value: "8081"
- name: IMAGE_PROVIDER_HOST
value: '{{ include "otel-demo.name" . }}-IMAGE_PROVIDER_PORT'

Choose a reason for hiding this comment

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

Suggested change
value: '{{ include "otel-demo.name" . }}-IMAGE_PROVIDER_PORT'
value: '{{ include "otel-demo.name" . }}-imageprovider'

graphaelli added a commit to graphaelli/opentelemetry-helm-charts that referenced this pull request Apr 17, 2024
@zeitlinger
Copy link
Member

IMAGE_PROVIDER_PORT is already required in https://github.com/grafana/opentelemetry-demo/blob/06023dd91e02d2da7818137b1e09eadd1d86228b/src/frontendproxy/envoy.tmpl.yaml#L109-L110 - so helm chart 0.30.2 is broken

@julianocosta89
Copy link
Member

IMAGE_PROVIDER_PORT is already required in https://github.com/grafana/opentelemetry-demo/blob/06023dd91e02d2da7818137b1e09eadd1d86228b/src/frontendproxy/envoy.tmpl.yaml#L109-L110 - so helm chart 0.30.2 is broken

Actually not @zeitlinger.
The Helm Chart has the image versions pinned to the last release, and imageProvider was not available on 1.9.0.

@zeitlinger
Copy link
Member

Actually not @zeitlinger. The Helm Chart has the image versions pinned to the last release, and imageProvider was not available on 1.9.0.

I'm using a fork of the otel demo - and did a rebase before I encountered the problem. Maybe that was the root cause.

graphaelli added a commit to graphaelli/opentelemetry-helm-charts that referenced this pull request Apr 24, 2024
…mageprovider

# Conflicts:
#	charts/opentelemetry-demo/Chart.yaml
#	charts/opentelemetry-demo/examples/bring-your-own-observability/rendered/component.yaml
#	charts/opentelemetry-demo/examples/bring-your-own-observability/rendered/flagd-config.yaml
#	charts/opentelemetry-demo/examples/bring-your-own-observability/rendered/serviceaccount.yaml
#	charts/opentelemetry-demo/examples/collector-as-daemonset/rendered/component.yaml
#	charts/opentelemetry-demo/examples/collector-as-daemonset/rendered/flagd-config.yaml
#	charts/opentelemetry-demo/examples/collector-as-daemonset/rendered/grafana-dashboards.yaml
#	charts/opentelemetry-demo/examples/collector-as-daemonset/rendered/serviceaccount.yaml
#	charts/opentelemetry-demo/examples/custom-environment-variables/rendered/component.yaml
#	charts/opentelemetry-demo/examples/custom-environment-variables/rendered/flagd-config.yaml
#	charts/opentelemetry-demo/examples/custom-environment-variables/rendered/grafana-dashboards.yaml
#	charts/opentelemetry-demo/examples/custom-environment-variables/rendered/serviceaccount.yaml
#	charts/opentelemetry-demo/examples/default/rendered/component.yaml
#	charts/opentelemetry-demo/examples/default/rendered/flagd-config.yaml
#	charts/opentelemetry-demo/examples/default/rendered/grafana-dashboards.yaml
#	charts/opentelemetry-demo/examples/default/rendered/serviceaccount.yaml
#	charts/opentelemetry-demo/examples/kubernetes-infra-monitoring/rendered/component.yaml
#	charts/opentelemetry-demo/examples/kubernetes-infra-monitoring/rendered/flagd-config.yaml
#	charts/opentelemetry-demo/examples/kubernetes-infra-monitoring/rendered/grafana-dashboards.yaml
#	charts/opentelemetry-demo/examples/kubernetes-infra-monitoring/rendered/serviceaccount.yaml
#	charts/opentelemetry-demo/examples/public-hosted-ingress/rendered/component.yaml
#	charts/opentelemetry-demo/examples/public-hosted-ingress/rendered/flagd-config.yaml
#	charts/opentelemetry-demo/examples/public-hosted-ingress/rendered/grafana-dashboards.yaml
#	charts/opentelemetry-demo/examples/public-hosted-ingress/rendered/serviceaccount.yaml
Copy link

github-actions bot commented May 7, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label May 7, 2024
@TylerHelmuth
Copy link
Member

Has the demo had a release yet with the new service?

@graphaelli
Copy link

No afaict, 1.9.0 was released 2 days before open-telemetry/opentelemetry-demo#1462 was merged

@github-actions github-actions bot removed the Stale label May 8, 2024
@julianocosta89
Copy link
Member

No afaict, 1.9.0 was released 2 days before open-telemetry/opentelemetry-demo#1462 was merged

Correct, imageprovider was not released yet.

@puckpuck puckpuck changed the title Demo imageprovider [demo] add imageprovider May 11, 2024
Signed-off-by: Pierre Tessier <pierre@pierretessier.com>
@puckpuck
Copy link
Contributor

@klucsik I needed to get this running for my team's demo efforts. I figured out why the nginx pod wasn't starting (securityContext) and updated the frontendproxy config. I pushed the changes to your branch.

I will push to get a demo 1.10.x release done, so we can merge this.

Copy link

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label May 26, 2024
@puckpuck puckpuck removed the Stale label Jun 3, 2024
@puckpuck puckpuck mentioned this pull request Jun 7, 2024
@puckpuck
Copy link
Contributor

With #1220 getting merged, this is no longer needed.

@puckpuck puckpuck closed this Jun 11, 2024
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.

6 participants