🐛 fix: (helm/v1alpha1): install the prometheus-operator CRDs before installing a chart which configures a ServiceMonitor #4383

monteiro-renato · 2024-11-22T18:04:22Z

k8s-ci-robot · 2024-11-22T18:04:32Z

Hi @monteiro-renato. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

monteiro-renato · 2024-11-22T20:15:54Z

.github/workflows/test-helm-samples.yml

+
+      - name: Check Presence of ServiceMonitor
+        run: |
+          kubectl wait --namespace project-v4-with-plugins-system --for=jsonpath='{.kind}'=ServiceMonitor servicemonitor/project-v4-with-plugins-controller-manager-metrics-monitor


Fails with exit 1 code if the resource does not exist.

I think we have a bug.
For we are able to check it out we need enable prometheus in the HelmChart value.

By default it is

prometheus: enable: false

we need to use shell to change it in the GitHUb action to set true
Something like example: https://github.com/kubernetes-sigs/kubebuilder/blob/master/.github/workflows/test-e2e-samples.yml#L43-L44

Then, the error will be faced
Not sure if the error is with chart manifests or in the action:
pkg/plugins/optional/helm/v1alpha/scaffolds/internal/templates/github/test_chart.go

I'm passing the value to the helm install command in line 78 :)
Check the --set flag of the helm install

nit: how about adding an explicit timeout flag for kubectl wait:

--timeout=60s

Also, --for='' could be another choice to consider?

This is a bit of an hack since the kubectl wait is supposed to be used with resources that have a Status field.
The command fails right away if the resource doesn't exist so the --timeout flag isn't really needed here (I guess --timeout=0 would be ok).

If kubectl get on a non existent resource would somehow return a non zero exit code that's what I would have used but I don't think that's possible. And between processing the output of kubectl get and simply using the hack above, I think the hack above is easier to understand.

The --for='' could be an option yea; my thought was that it could lead to confusion as it might be less clear.

Kavinjsir

Looks good to me!

Kavinjsir · 2024-11-23T04:55:30Z

.github/workflows/test-helm-samples.yml

+
+      - name: Check Presence of ServiceMonitor
+        run: |
+          kubectl wait --namespace project-v4-with-plugins-system --for=jsonpath='{.kind}'=ServiceMonitor servicemonitor/project-v4-with-plugins-controller-manager-metrics-monitor


nit: how about adding an explicit timeout flag for kubectl wait:

--timeout=60s

Also, --for='' could be another choice to consider?

camilamacedo86

Hi @monteiro-renato

We also need to change the github action taht we are scaffolding for the projects, see; pkg/plugins/optional/helm/v1alpha/scaffolds/internal/templates/github/test_chart.go

camilamacedo86 · 2024-11-23T15:06:34Z

pkg/plugins/optional/helm/v1alpha/scaffolds/internal/templates/github/test_chart.go

@@ -118,9 +124,13 @@ jobs:

      - name: Install Helm chart for project
        run: |
-          helm install my-release ./dist/chart --create-namespace --namespace {{ .ProjectName }}-system
+          helm install my-release ./dist/chart --create-namespace --namespace {{ .ProjectName }}-system --set prometheus.enable=true


See, we cannot add this one here.
Because the Prometheus is disabled by default
If the person enables Prometheus in the chart and commits it.
Then, it will be enabled already, or if the author prefers, then it can be done.

Is that make sense?

Yep updated it now 👍

camilamacedo86

Just I nit all the rest shows great 🥇
Terrific work

See: https://github.com/kubernetes-sigs/kubebuilder/pull/4383/files#r1855209081

camilamacedo86 · 2024-11-24T09:20:38Z

testdata/project-v4-with-plugins/.github/workflows/test-chart.yml

+          helm repo add prometheus-community https://prometheus-community.github.io/helm-charts
+          helm repo update
+          helm install prometheus-crds prometheus-community/prometheus-operator-crds
+


@monteiro-renato
this one needs to be commented on under # TODO: Uncomment if Prometheus is enabled
We should only install it if the chart enabled the option

I thought it would be a bit easier to just install the CRDs all the time as they shouldn't add too much time to the action run.

I've shifted them to be under the TODO now 👍.

…nstalling a chart which configures a ServiceMonitor

camilamacedo86

/lgtm
/approved
/ok-to-test

k8s-ci-robot · 2024-11-24T12:07:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, Kavinjsir, monteiro-renato

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Kavinjsir,camilamacedo86]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 22, 2024

k8s-ci-robot requested review from Kavinjsir and varshaprasad96 November 22, 2024 18:04

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 22, 2024

monteiro-renato marked this pull request as draft November 22, 2024 18:40

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 22, 2024

monteiro-renato force-pushed the patch-4378 branch from 8f901f0 to fd809e1 Compare November 22, 2024 18:55

monteiro-renato force-pushed the patch-4378 branch from aa7a43f to 42940d4 Compare November 22, 2024 19:45

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 22, 2024

monteiro-renato force-pushed the patch-4378 branch 2 times, most recently from aba0330 to e545894 Compare November 22, 2024 20:12

monteiro-renato commented Nov 22, 2024

View reviewed changes

monteiro-renato marked this pull request as ready for review November 22, 2024 20:16

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 22, 2024

Kavinjsir approved these changes Nov 23, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 23, 2024

camilamacedo86 reviewed Nov 23, 2024

View reviewed changes

monteiro-renato force-pushed the patch-4378 branch from e545894 to ed95269 Compare November 23, 2024 13:34

k8s-ci-robot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 23, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 23, 2024

camilamacedo86 reviewed Nov 23, 2024

View reviewed changes

camilamacedo86 requested changes Nov 23, 2024

View reviewed changes

camilamacedo86 self-requested a review November 23, 2024 15:16

monteiro-renato force-pushed the patch-4378 branch from ed95269 to 35903fd Compare November 23, 2024 15:34

camilamacedo86 reviewed Nov 24, 2024

View reviewed changes

🐛 fix: (helm/v1alpha1): install the prometheus-operator CRDs before i…

256de94

…nstalling a chart which configures a ServiceMonitor

monteiro-renato force-pushed the patch-4378 branch from 35903fd to 256de94 Compare November 24, 2024 09:31

camilamacedo86 approved these changes Nov 24, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 24, 2024

k8s-ci-robot assigned camilamacedo86 Nov 24, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 24, 2024

k8s-ci-robot merged commit 8911e2f into kubernetes-sigs:master Nov 24, 2024
22 checks passed

monteiro-renato deleted the patch-4378 branch November 24, 2024 12:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 fix: (helm/v1alpha1): install the prometheus-operator CRDs before installing a chart which configures a ServiceMonitor #4383

🐛 fix: (helm/v1alpha1): install the prometheus-operator CRDs before installing a chart which configures a ServiceMonitor #4383

monteiro-renato commented Nov 22, 2024

k8s-ci-robot commented Nov 22, 2024

monteiro-renato Nov 22, 2024

camilamacedo86 Nov 22, 2024 •

edited

Loading

monteiro-renato Nov 22, 2024

Kavinjsir Nov 23, 2024

monteiro-renato Nov 23, 2024

Kavinjsir left a comment

Kavinjsir Nov 23, 2024

camilamacedo86 left a comment •

edited

Loading

camilamacedo86 Nov 23, 2024

monteiro-renato Nov 23, 2024

camilamacedo86 left a comment

camilamacedo86 Nov 24, 2024 •

edited

Loading

monteiro-renato Nov 24, 2024

camilamacedo86 left a comment

k8s-ci-robot commented Nov 24, 2024

🐛 fix: (helm/v1alpha1): install the prometheus-operator CRDs before installing a chart which configures a ServiceMonitor #4383

🐛 fix: (helm/v1alpha1): install the prometheus-operator CRDs before installing a chart which configures a ServiceMonitor #4383

Conversation

monteiro-renato commented Nov 22, 2024

k8s-ci-robot commented Nov 22, 2024

Choose a reason for hiding this comment

camilamacedo86 Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kavinjsir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

camilamacedo86 Nov 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 24, 2024

camilamacedo86 Nov 22, 2024 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

camilamacedo86 Nov 24, 2024 •

edited

Loading