-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add option to select keda release to KedaController CRD #217
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach is looking good.
@@ -9,6 +9,8 @@ spec: | |||
# with Name set to 'keda' created in namespace 'keda' | |||
### | |||
|
|||
# kedaRelease: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add please add here comment with a specific example?
@@ -11,14 +12,17 @@ const resourcesPath = "keda.yaml" | |||
const olmResourcesPath = "keda-olm-operator.yaml" | |||
const LastConfigID = "olm-operator.keda.sh/last-applied-configuration" | |||
|
|||
func GetResourcesManifest() (mf.Manifest, error) { | |||
func GetResourcesManifest(kedaRelease string) (mf.Manifest, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we please add a test and a validation that kedaRelease
is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to use semver
package to do the test, but there's an issue. Keda releases on GH are versioned using semantic versioning scheme, but the build arg used during docker build is not (missing 'v'
prefix). See for example in keda logs:
2024-02-20T13:20:20Z INFO setup KEDA Version: 2.12.0
Should users use kedaRelease: "v2.12.1"
or kedaRelease: "2.12.1"
?
Or should we add the v
if it's not present?
This will also determin the dir structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would go with both options, default (documented) would be kedaRelease: "2.12.1"
and prepend v
manually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another note - do we allow updating and downgrading via updates on KedaController
? If so, this should be tested.
I manually tried to move forward and backwards between releases v2.10.1, v2.12.0 and v2.12.1 and KEDA, metrics + webhook servers were deployed successfully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also update Readme with this new propery (add example and expected input there, pls).
https://github.com/kedacore/keda-olm-operator?tab=readme-ov-file#the-kedacontroller-custom-resource
@joelsmith is this change okay with you? |
Signed-off-by: Josef Karasek <[email protected]>
Signed-off-by: Josef Karasek <[email protected]>
Signed-off-by: Josef Karasek <[email protected]>
Signed-off-by: Josef Karasek <[email protected]>
Signed-off-by: Josef Karasek <[email protected]>
Signed-off-by: Josef Karasek <[email protected]>
Signed-off-by: Josef Karasek <[email protected]>
58436e2
to
ac1d3de
Compare
Signed-off-by: Josef Karasek <[email protected]>
Signed-off-by: Josef Karasek <[email protected]>
@joelsmith @jkyros are you comfortable with this change please? |
I am fine with the concept of the change, although I believe we would want to add a way to disable this feature for distributors who want the versions to be more tightly coupled (i.e. preserve current behavior). However, this PR makes a lot of changes that I was not expecting to see when I read the PR title. I will need to allocate time in my schedule to do a thorough review of the changes to the manifest handling, etc. |
Sounds good, how would you prefer to add the option to disable this feature? ENV variable or a parameter? Or even a build-time option? |
@joelsmith have you got a chance to look at this please? |
Closing this PR. I went over the design with Zbynek again and we're worried about the amount of time thaw would be spent on testing that upgrades work. We need to go back to the drawing board for a bit. |
New config option
kedaRelease
inkedacontroller.spec
. The intention is to use the operator to deploy specific keda release.If accepted, multiple manifest versions/releases will need to be baked into the image. For example last three KEDA releases.
Checklist