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

Enable configuring PDB from Limitador CR #91

Merged
merged 4 commits into from
Aug 30, 2023

Conversation

adam-cattermole
Copy link
Member

@adam-cattermole adam-cattermole commented Aug 14, 2023

Added ability to configure PodDisruptionBudget from Limitador CR.

Defining a limitador CR with the pdb set results in an associated PodDisruptionBudget object to be created.

apiVersion: limitador.kuadrant.io/v1alpha1
kind: Limitador
metadata:
  name: limitador-sample
spec:
  pdb:
    maxUnavailable: 1
kind: PodDisruptionBudget
apiVersion: policy/v1
metadata:
  name: limitador-limitador-sample
spec:
  selector:
    matchLabels:
      app: limitador
  maxUnavailable: 1

Also added new make target make local-redeploy that re-builds image locally and rolls out deployment in kind cluster with the local changes.

Resolves #75

@codecov-commenter
Copy link

codecov-commenter commented Aug 14, 2023

Codecov Report

Merging #91 (ad29718) into main (e110bb7) will increase coverage by 0.52%.
The diff coverage is 49.46%.

@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
+ Coverage   43.31%   43.83%   +0.52%     
==========================================
  Files          11       12       +1     
  Lines         718      803      +85     
==========================================
+ Hits          311      352      +41     
- Misses        369      408      +39     
- Partials       38       43       +5     
Flag Coverage Δ
unit 43.83% <49.46%> (+0.52%) ⬆️

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

Files Changed Coverage Δ
pkg/reconcilers/base_reconciler.go 34.54% <0.00%> (-0.64%) ⬇️
pkg/reconcilers/poddisruptionbudget.go 0.00% <0.00%> (ø)
controllers/limitador_controller.go 42.05% <44.73%> (+0.57%) ⬆️
pkg/limitador/k8s_objects.go 71.42% <87.87%> (+3.74%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@adam-cattermole adam-cattermole force-pushed the reconcile-pdb branch 2 times, most recently from a38a173 to 901d7a7 Compare August 14, 2023 12:04
@adam-cattermole adam-cattermole added kind/enhancement New feature or request participation/good first issue Good for newcomers target/current size/small area/api CRD or other public API related labels Aug 14, 2023
@adam-cattermole adam-cattermole changed the title Enable configuring PDB from Limitador CR [WIP] -Enable configuring PDB from Limitador CR Aug 14, 2023
@adam-cattermole adam-cattermole marked this pull request as draft August 14, 2023 12:46
@adam-cattermole adam-cattermole marked this pull request as ready for review August 14, 2023 13:35
@adam-cattermole adam-cattermole changed the title [WIP] -Enable configuring PDB from Limitador CR Enable configuring PDB from Limitador CR Aug 14, 2023
Copy link
Contributor

@Boomatang Boomatang left a comment

Choose a reason for hiding this comment

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

Happy path works but I did find two issue that will need to be addressed.

  1. The user is allowed to state the selector fields in the limitador CR but this ignored by the operator. In this case it might better to not allow the user state the selector fields.
  2. The selector field of app is not unique and would apply to multiply limitador CR deploy of which all may have different conflicting PDB polices.

There are a few smaller comments but that is the big points.

Makefile Outdated Show resolved Hide resolved
controllers/limitador_controller.go Outdated Show resolved Hide resolved
controllers/limitador_controller.go Show resolved Hide resolved
pkg/reconcilers/poddisruptionbudget.go Show resolved Hide resolved
pkg/reconcilers/poddisruptionbudget.go Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
api/v1alpha1/limitador_types.go Outdated Show resolved Hide resolved
controllers/limitador_controller.go Outdated Show resolved Hide resolved
Co-authored-by: Grzegorz Piotrowski <[email protected]>
Copy link
Contributor

@Boomatang Boomatang left a comment

Choose a reason for hiding this comment

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

All the issue and change request have being addressed. I would be happy to see this merged.

@adam-cattermole adam-cattermole merged commit 972d597 into Kuadrant:main Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api CRD or other public API related kind/enhancement New feature or request participation/good first issue Good for newcomers size/small target/current
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Production-ready: Configure PDB
4 participants