feat: Move http scaled object from single host to multi host system

[someshkoli]

Signed-off-by: Somesh Koli somesh.koli@headout.com

Initial tests, Moving http scaled object from single host association to multi host.
Breaking schema change.

Checklist

• Commits are signed with Developer Certificate of Origin (DCO)
• Any necessary documentation is added, such as:
  • README.md
  • The docs/ directory
  • The docs repo
• Breaking schema change, update docs and merge carefully.

Fixes #552

[JorTurFer]

Looking good. Thanks for the contribution ❤️
e2e tests is broken due to this breaking change, you need to update the example xkcd chart also with this change. We use it as part of e2e tests and that's why they are failing.
About the breaking change, @tomkerkhove , is this allowed for the add-on or should we maintain both cases (at least some time)?

[tomkerkhove]

We should only do breaking changes in 1.0 IMO; but this might be a good example where we can bump API version and support both in 0.5.0 :)

[tomkerkhove]

We should only do breaking changes in 1.0 IMO; but this might be a good example where we can bump API version and support both in 0.5.0 :)

@someshkoli would you mind doing the change in a non-breaking change?

[someshkoli]

We should only do breaking changes in 1.0 IMO; but this might be a good example where we can bump API version and support both in 0.5.0 :)

@someshkoli would you mind doing the change in a non-breaking change?

Can do, but it'll create a bit of redundancy to support backward compatibility.
Do we want to go on that path?

[tomkerkhove]

Meh it's still in preview - Let's just break things :)

[tomkerkhove]

Can you document the breaking change in our new changelog please?

[someshkoli]

Will do, need to run e2e, @tomkerkhove can you update status for that ?
Also can I trigger them on my own ?

[tomkerkhove]

Can you rebase please?

[someshkoli]

Okay looks like there has been a major revamp in types :") have a lot to rebase

[JorTurFer]

LGTM!

[JorTurFer]

I didn't see the conflicts, :(
Could you rebase the PR please?

[someshkoli]

I didn't see the conflicts, :(
Could you rebase the PR please?

I do see some big conflicts while rebasing, mostly because hosts has been moved from spec to triggers.

[JorTurFer]

I see these:

[someshkoli]

I see these:

Yeah, they have some good level of refactor to httpscaled object. Will try to onboard these changes.

[OdyX]

Any news on that front? Would you need help rebasing?

[someshkoli]

Any news on that front? Would you need help rebasing?

Have been quite occupied lately, would appreciate help with rebase. lmk if you'd like to connect and get this done ?

[jocelynthode]

Hey @someshkoli and @JorTurFer I've started working on the rebase here: https://github.com/jocelynthode/http-add-on/pull/1/files.

However, I noticed some things in the PR which I think might cause issues. For example when defining the ScaledObject you opted to go with metadata.hosts being an array of hosts just like the HTTPScaledObject. However KEDA type struct for ScaledObject only allows types map[string]string for metadata field. I think we should instead define multiple triggers, having one for each host.

This would let us respect the struct of KEDA. I've started in the above mentioned PR this refactoring. Please let me know if I missed something and this is a non issue.

Edit: Actually by investigating a bit more it looks like this metadata is only a metadata (cf https://github.com/kedacore/keda/blob/cf3cd1dc49bd77e86d57e8a7b12689f70dd91348/pkg/scalers/external_scaler.go#L104 where only scalerAddress and tlsCertFile actually do something) so I opted to concatenate the array to a comma separated string. Let me know if this can't work.

[JorTurFer]

Edit: Actually by investigating a bit more it looks like this metadata is only a metadata (cf kedacore/keda@cf3cd1d/pkg/scalers/external_scaler.go#L104 where only scalerAddress and tlsCertFile actually do something) so I opted to concatenate the array to a comma separated string. Let me know if this can't work.

You can provide all the elements that you want:
https://github.com/kedacore/keda/blob/4ae8975af4a2a43d94e66b0b7be895e189312d39/pkg/scalers/external_scaler.go#L142-L151

[jocelynthode]

Edit: Actually by investigating a bit more it looks like this metadata is only a metadata (cf kedacore/keda@cf3cd1d/pkg/scalers/external_scaler.go#L104 where only scalerAddress and tlsCertFile actually do something) so I opted to concatenate the array to a comma separated string. Let me know if this can't work.

You can provide all the elements that you want: https://github.com/kedacore/keda/blob/4ae8975af4a2a43d94e66b0b7be895e189312d39/pkg/scalers/external_scaler.go#L142-L151

Yes but they must fit in map[string]string which made the solution in this PR unfeasible as the metadata field value was not a string but an array.

[JorTurFer]

I close this PR in favor of the other