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: use proper image versions in install manifest #1010

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pintohutch
Copy link
Collaborator

Bump the operator-related images up to v0.10.0-gke.6.

Bump the prometheus and alertmanager images as well.

Bump the operator-related images up to v0.10.0-gke.6.

Bump the prometheus and alertmanager images as well.
Copy link
Member

@TheSpiritXIII TheSpiritXIII left a comment

Choose a reason for hiding this comment

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

Thanks!

alertmanager:
image: gke.gcr.io/prometheus-engine/alertmanager
tag: v0.25.1-gmp.2-gke.0
tag: v0.25.1-gmp.3-gke.0
prometheus:
# TODO(bwplotka): Change to "v2.45.3-gmp.4-gke.0" once tags are cloned.
Copy link
Member

Choose a reason for hiding this comment

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

nit: Can you update the tag in this comment? So it's clear what the SHA points to?

datasourceSyncer:
image: gcr.io/gke-release/prometheus-engine/datasource-syncer
image: gke.gcr.io/prometheus-engine/datasource-syncer
#TODO(macxamin) Sync CURRENT_DATASOURCE_SYNCER_TAG with CURRENT_TAG
Copy link
Collaborator

@maxamins maxamins Jun 10, 2024

Choose a reason for hiding this comment

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

This comment can be removed with this change.

@@ -534,7 +534,7 @@ spec:
priorityClassName: gmp-critical
containers:
- name: operator
image: gke.gcr.io/prometheus-engine/operator:v0.9.0-gke.1
image: gke.gcr.io/prometheus-engine/operator:v0.10.0-gke.6
Copy link
Collaborator

Choose a reason for hiding this comment

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

I doubt this works due to #1013

(with readOnlyRootFilesystem: true)

Have you tested it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes - indeed #944 breaks this.

@bernot-dev or @TheSpiritXIII do you know if we have an image published we could use that works with that change? Presumably HEAD worked (v0.13.0-rc.0).

OTOH maybe trying to "fix" our nightly manifest in this way isn't a great idea and I should abandon the PR as it isn't possible to always have a working manifest in HEAD anyway due to GKE build pipeline delays...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean, you are not making it worse so let's go =D

Good point that we could have issues going forward, but we have same problem with AM and Prometheus and for those we HAVE to make them workable in atomic way otherwise tests won't pass. It's just with PE images we are "allowed" to cheat a bit.

I wonder if splitting manifests to OSS official ones VS source for managed GMP should be different things e.g. for GMP we could simply use helm templates directly and have different values (: We could actually get rid of kustomize (or move kustomize here). cc @TheSpiritXIII

Copy link
Collaborator

Choose a reason for hiding this comment

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

The alternative (or actually doing both ideas might work) would be to have official "dev" registry we will push ALWAYS and register the version here somehow (maybe "main" tag after all). Not tested, but it will be builded like any other image. Then we can promote to official prod registry and tag on the final release.

Copy link
Collaborator

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

LGTM, mod comments.

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.

None yet

4 participants