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

Gh 668 #143

Merged
merged 1 commit into from
Jul 2, 2024
Merged

Gh 668 #143

merged 1 commit into from
Jul 2, 2024

Conversation

R-Lawton
Copy link
Contributor

@R-Lawton R-Lawton commented Jun 9, 2024

WHAT:
Adding labels to resources as limitador often runs in the same namespace as other operators so we are seeing certain issues with metrics repeating mainly around the service resource. Since its adding a label it shouldn't break things

@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.40%. Comparing base (da27315) to head (afbcffe).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #143      +/-   ##
==========================================
- Coverage   85.01%   84.40%   -0.61%     
==========================================
  Files          19       19              
  Lines         994      994              
==========================================
- Hits          845      839       -6     
- Misses         97      100       +3     
- Partials       52       55       +3     
Flag Coverage Δ
integration 78.37% <ø> (-1.01%) ⬇️
unit 66.51% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
api/v1alpha1 (u) 100.00% <ø> (ø)
pkg/helpers (u) 83.87% <ø> (ø)
pkg/log (u) 94.73% <ø> (ø)
pkg/reconcilers (u) 74.67% <ø> (ø)
pkg/limitador (u) 98.11% <ø> (ø)
controllers (i) 73.33% <ø> (-2.00%) ⬇️
pkg/upgrades 88.88% <ø> (ø)

see 2 files with indirect coverage changes

@david-martin
Copy link
Member

This change is needed as the limitador operator typically runs in the same namespace as other operators when deployed as part of kuadrant.
The change should be backwards compatible as it's adding a new label app: limitador to various resources.
The most important one is the metrics Service, as it can result in odd behaviour where it routes traffic to more than just the limitador operator.

@david-martin
Copy link
Member

@eguzki thoughts on this change?
Is it sufficient to make the changes here so that kuadrant-operator deploys the resources with the new label?

@R-Lawton
Copy link
Contributor Author

Hey @eguzki, What do you think of these changes am I missing anything?

Copy link
Contributor

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

Overall looks good.

One question regarding the labels left.

@david-martin yes, it is sufficient to make the changes here so that kuadrant-operator deploys the resources with the new label

config/manager/manager.yaml Outdated Show resolved Hide resolved
config/rbac/auth_proxy_service.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

Other than a tiny issue with the createdAt field, LGTM. But it needs rebase first before approval

@R-Lawton R-Lawton force-pushed the gh-668 branch 3 times, most recently from 6befa9c to c07e895 Compare July 2, 2024 10:26
@R-Lawton R-Lawton merged commit e3f28fc into Kuadrant:main Jul 2, 2024
10 checks passed
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.

4 participants