Reconcile the pyrra generated prometheusrule crs with azure prometheusrulegroups resources #1110

monteiro-renato · 2024-11-13T11:41:13Z

Related Issue(s)

Reconcile the Pyrra generated PrometheusRule CRs with Azure PrometheusRuleGroups resources #997
Create the Terraform code needed to give the controller the permissions it needs to reconcile PrometheusRuleGroups resources #1103

Verification

Your code builds clean without any errors or warnings
Manual testing done (required)
Relevant automated test added (if you find this hard, leave it and we'll help out)
All tests run green

kubebuilder init --domain dis.altinn.cloud --repo github.com/Altinn/altinn-platform/services/dis-promrulegroups-operator

make build-installer

kubebuilder create api --group monitoring.coreos.com --version v1 --kind PrometheusRule --controller --resource=false --external-api-path="github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" make build-installer

rename monitoringcoreoscomv1 to monitoringv1 make build-installer

basic tests go mod tidy

coderabbitai · 2024-11-13T11:41:22Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

sduranc · 2024-11-14T13:03:31Z

services/dis-promrulegroups-operator/dist/install.yaml

@@ -0,0 +1,259 @@
+apiVersion: v1


was the dist folder pushed by mistake?

Intentional, it's to show which changes I made to what's generated.

sduranc · 2024-11-14T13:08:03Z

Wow, this is a long PR. Not sure I'll get time this week to review it. I find the name a bit odd (yeah, naming's hard), It makes more sense to have a "wider" operator and include this as a controller, e.g slo operator, observability operator, etc.

monteiro-renato · 2024-11-14T18:31:31Z

I made sure to make multiple commits to make it easier to review.
You can ignore the first three commits as that's all generated stuff.
The fourth commit onwards show the actual changes I've made.

monteiro-renato · 2024-11-14T18:36:00Z

Regarding the naming, I think we should keep this controller separate from other eventual controllers we build. This is mostly to address a limitation we've faced when moving from kube-prometheus to the Azure managed Prometheus. Ideally, Azure will provide a native way to do this in the future and we can simply stop using this controller.

… monitoring subscription

tjololo

Sorry for the lat review! Commented some of my thoughts

services/dis-promrulegroups-operator/Dockerfile.bak

tjololo · 2024-11-25T09:41:38Z

services/dis-promrulegroups-operator/cmd/main.go

+	if os.Getenv("ENVIRONMENT") == "" {
+		err := godotenv.Load()
+		if err != nil {
+			setupLog.Error(err, "Error loading .env file")
+			os.Exit(1)
+		}
+	}


This is a NIT!
I have used koanf in my operator, but the work is not worth it if we think this operator is going to be undeployed soon.
My koanf config loading: https://github.com/Altinn/altinn-platform/blob/main/services/dis-apim-operator/internal/config/conf.go

tjololo · 2024-11-25T11:09:32Z

services/dis-promrulegroups-operator/.github/workflows/lint.yml

add this workflows to the repo workflows folder so they get triggered. Remember to add path limitations so it only run when changes to this operator or pipeline is pushed

tjololo · 2024-11-25T11:10:04Z

services/dis-promrulegroups-operator/.github/workflows/test-e2e.yml

add this workflows to the repo workflows folder, if e2e tests are implemented, so they get triggered. Remember to add path limitations so it only run when changes to this operator or pipeline is pushed

tjololo · 2024-11-25T11:10:10Z

services/dis-promrulegroups-operator/.github/workflows/test.yml

add this workflows to the repo workflows folder so they get triggered. Remember to add path limitations so it only run when changes to this operator or pipeline is pushed

services/dis-promrulegroups-operator/internal/controller/prometheusrule_controller.go

tjololo · 2024-11-25T11:52:46Z

services/dis-promrulegroups-operator/internal/controller/prometheusrule_controller.go

+		for _, rn := range strings.Split(resourceNames, ",") {
+			_, err := r.deletePrometheusRuleGroup(ctx, rn)
+			if err != nil {
+				// TODO: Should we try to delete the rest in case one deletion fails? Or simply retry again?


Might be good idea to try to delete the rest and then return an err. If we just retry/requeue this would block all the resources later in the slice

Fixed here: fd2b33c

services/dis-promrulegroups-operator/internal/controller/prometheusrule_controller.go

tjololo

One small question and a nit, other than that it looks good.
Is this still a draft?

tjololo · 2024-12-06T13:12:37Z

services/dis-promrulegroups-operator/dist/chart/templates/certmanager/certificate.yaml

+    name: selfsigned-issuer
+  secretName: webhook-server-cert
+{{- end }}
+{{- if and .Values.metrics.enable .Values.certmanager.enable }}


NIT
Probably generated code, the check .Values.certmanager.enable seems unnecessary as the outer most if already has checked that it is true

Generated code indeed.
One of the {{end}} blocks should be further up.

services/dis-promrulegroups-operator/internal/controller/prometheusrule_controller.go

services/dis-promrulegroups-operator/Dockerfile

monteiro-renato · 2024-12-10T12:06:02Z

One small question and a nit, other than that it looks good. Is this still a draft?

I guess it's not a draft anymore; I've tested it by deploying manually to two AT clusters (I cleaned up after myself don't worry :)).
Do we have any place I can use to push the image and the helm chart to?
If I can do that, I can use the helm provider and deploy this to the AT environments to get some "real world" testing.

tjololo · 2024-12-10T12:12:55Z

One small question and a nit, other than that it looks good. Is this still a draft?

I guess it's not a draft anymore; I've tested it by deploying manually to two AT clusters (I cleaned up after myself don't worry :)). Do we have any place I can use to push the image and the helm chart to? If I can do that, I can use the helm provider and deploy this to the AT environments to get some "real world" testing.

I suggest we use the github registry and then push to a private registry as well, or use a private cache for the Image.
We have a repo for other helm charts here: github.com/Altinn/altinn-studio-charts but that don't seem like a good match. Is it possible to use ghcr.io for helm charts as well?

monteiro-renato · 2024-12-10T12:53:14Z

One small question and a nit, other than that it looks good. Is this still a draft?

I guess it's not a draft anymore; I've tested it by deploying manually to two AT clusters (I cleaned up after myself don't worry :)). Do we have any place I can use to push the image and the helm chart to? If I can do that, I can use the helm provider and deploy this to the AT environments to get some "real world" testing.

I suggest we use the github registry and then push to a private registry as well, or use a private cache for the Image. We have a repo for other helm charts here: github.com/Altinn/altinn-studio-charts but that don't seem like a good match. Is it possible to use ghcr.io for helm charts as well?

Never actually tried it but I suppose it should be possible yea. I, however, don't have the required permissions to make the setup work. This would require us to make configuration changes in Github and in our Terraform pipelines. I guess no one with the needed permissions would have the time to work on this now 😅 .

monteiro-renato added 16 commits November 13, 2024 07:51

scaffold the project

c87a612

kubebuilder init --domain dis.altinn.cloud --repo github.com/Altinn/altinn-platform/services/dis-promrulegroups-operator

generate the dist folder

63151d5

make build-installer

create the controller

7728238

kubebuilder create api --group monitoring.coreos.com --version v1 --kind PrometheusRule --controller --resource=false --external-api-path="github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" make build-installer

update the rbac permissions

28396c7

rename monitoringcoreoscomv1 to monitoringv1 make build-installer

update Dockerfile to include the Azure tool

10e8438

update the README

f3f6f2d

examples of input and output of the conversion tool

a1d5ee2

add npm and az-prom-rules-converter as dependencies

13fea12

include the PrometheusRule CRD

fa10f01

add an example .env file

afd7fd8

setup the controller dependencies

48db0e5

only resources in the monitoring namespace

e0cf42a

include the PrometheusRule CRD in the test suite

50b824f

add a file with test utils

f138a09

first implementation

04aac84

basic tests go mod tidy

k8s deployment configs

da40ae5

monteiro-renato requested review from tjololo, khanrn and sduranc November 13, 2024 11:41

sduranc reviewed Nov 14, 2024

View reviewed changes

add a prefix to the rule names to make them unique in the centralized…

bd48b3b

… monitoring subscription

tjololo reviewed Nov 25, 2024

View reviewed changes

monteiro-renato added 4 commits November 26, 2024 16:16

kubebuilder edit --plugins=go/v4,helm/v1-alpha

0e3a647

helm template

1a2baa2

add .env to gitignore

730858b

update values.yaml

ff46f57

monteiro-renato added 9 commits November 28, 2024 12:49

Remove Dockerfile.bak

a1377b7

return error as nil when RequeueAfter is configured

c0066c9

simplify if condition

3227610

log errors as errors and not as info

6a25df2

simplify if else block

c325b96

rename variable to logger to avoid confusion with the package name

257877f

try to delete the rest of the resources even if one deletion fails

fd2b33c

return false as soon as the first missing annotation is seen

ad2cf3c

point to Azure repo now that the PRs have been merged

d164a3b

tjololo reviewed Dec 10, 2024

View reviewed changes

add new line at the end of the file

b83adad

monteiro-renato added 6 commits January 2, 2025 07:45

prefix promrulegroups workflow files

3adda5a

move promrulegroups workflow files to root .github directory

e48c06e

tweaks to workflow files

64426bd

release workflows

ab448a1

config working-directory in golangci-lint-action

0265059

set the default working-directory

67beed3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reconcile the pyrra generated prometheusrule crs with azure prometheusrulegroups resources #1110

Reconcile the pyrra generated prometheusrule crs with azure prometheusrulegroups resources #1110

monteiro-renato commented Nov 13, 2024 •

edited

Loading

coderabbitai bot commented Nov 13, 2024 •

edited

Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

Documentation and Community

sduranc Nov 14, 2024

monteiro-renato Nov 14, 2024

sduranc commented Nov 14, 2024

monteiro-renato commented Nov 14, 2024

monteiro-renato commented Nov 14, 2024

tjololo left a comment

tjololo Nov 25, 2024

tjololo Nov 25, 2024

tjololo Nov 25, 2024

tjololo Nov 25, 2024

tjololo Nov 25, 2024

monteiro-renato Nov 28, 2024

tjololo left a comment

tjololo Dec 6, 2024

monteiro-renato Dec 10, 2024

monteiro-renato commented Dec 10, 2024

tjololo commented Dec 10, 2024 •

edited

Loading

monteiro-renato commented Dec 10, 2024 •

edited

Loading

Reconcile the pyrra generated prometheusrule crs with azure prometheusrulegroups resources #1110

Are you sure you want to change the base?

Reconcile the pyrra generated prometheusrule crs with azure prometheusrulegroups resources #1110

Conversation

monteiro-renato commented Nov 13, 2024 • edited Loading

Related Issue(s)

Verification

coderabbitai bot commented Nov 13, 2024 • edited Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

Documentation and Community

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sduranc commented Nov 14, 2024

monteiro-renato commented Nov 14, 2024

monteiro-renato commented Nov 14, 2024

tjololo left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjololo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monteiro-renato commented Dec 10, 2024

tjololo commented Dec 10, 2024 • edited Loading

monteiro-renato commented Dec 10, 2024 • edited Loading

monteiro-renato commented Nov 13, 2024 •

edited

Loading

coderabbitai bot commented Nov 13, 2024 •

edited

Loading

tjololo commented Dec 10, 2024 •

edited

Loading

monteiro-renato commented Dec 10, 2024 •

edited

Loading