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

fix: Rename keda-http-add-on-interceptor-proxy to keda-add-ons-http-interceptor-proxy #1037

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kahirokunn
Copy link
Contributor

@kahirokunn kahirokunn commented May 21, 2024

Renamed keda-http-add-on-interceptor-proxy to keda-add-ons-http-interceptor-proxy to align with naming conventions as referenced in the source: https://github.com/kedacore/charts/blob/main/http-add-on/Chart.yaml#L3

Checklist

Fixes #

@kahirokunn kahirokunn requested a review from a team as a code owner May 21, 2024 05:13
@JorTurFer
Copy link
Member

Thanks for the PR 😄

This behaviour is documented here: https://github.com/kedacore/http-add-on/blob/main/docs/walkthrough.md#routing-to-the-right-service
image

Personally I'd prefer to wait until going to GA, but maybe @zroubalik and @tomkerkhove have other opinion

@kahirokunn
Copy link
Contributor Author

I was doing a TUTORIAL today and thought I couldn't connect, but now that you mention it I understand everything.

@tomkerkhove
Copy link
Member

Should we abandon the PR then?

@kahirokunn
Copy link
Contributor Author

I think users will be happier if it is merged because it doesn't actually work regardless of the warnings, but it depends on the project's policy, so if it is not acceptable, it can be discarded.

@zroubalik
Copy link
Member

We should use one naming, regardles what was the installation method, but I would use keda-http-add-on-interceptor-proxy not ...add-ons... so we follow the name of the repo.

@kahirokunn
Copy link
Contributor Author

@zroubalik Is this correct?
I would love to reduce the number of people who stumble across this wonderful add-on tutorial by one as soon as possible.

@wozniakjan
Copy link
Member

thank you @kahirokunn, I like your PR very much!

tbh, I also got a bit confused initially by the discrepancy in naming. I accepted that as a quirky behaviour of the chart and accepted that as is, but unifying the names regardless of the installation method would make me happier :)

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

We would also need to consolidate stuff in the Helm chart: https://github.com/kedacore/charts/tree/main/http-add-on

Let's see what @JorTurFer thinks

@kahirokunn
Copy link
Contributor Author

Thank you LGTM!

@JorTurFer
Copy link
Member

I see the benefit of unifying them, but I'm afraid about the impact.
Currently, that service is the touchpoint that users use for their ingresses and changing it will break all the integrations, IMHO we should wait until the going to GA change (which will include more breaking changes for sure) or at least until the dedicated ScalingSet are available (as the users can deploy a new ScalingSet with its own service that won't change).
If I'm the only one who see a problem with this, we can merge it and that's all, I can be overthinking xD

@zroubalik
Copy link
Member

I see the benefit of unifying them, but I'm afraid about the impact. Currently, that service is the touchpoint that users use for their ingresses and changing it will break all the integrations, IMHO we should wait until the going to GA change (which will include more breaking changes for sure) or at least until the dedicated ScalingSet are available (as the users can deploy a new ScalingSet with its own service that won't change). If I'm the only one who see a problem with this, we can merge it and that's all, I can be overthinking xD

I see the point, but I think the sooner we do the change the lesser impact, also this is still in beta, so we can do any breaking changes. Users will just need to migrate to the new version.

@JorTurFer
Copy link
Member

I see the point, but I think the sooner we do the change the lesser impact

Yes, totally. My point is that ScalingSet feature can offer an smooth migration path as they can just deploy their new ScalingSet and migrate the workloads to it as they can have both in parallel. Currently, they need to deploy another HTTP Add-on (all the components) temporally.
This have been as it is for a long, I think that a single extra version isn't a problem. Other option can be just maintaining the helm chart as it is (or even keeping both names) to not break integrations, deploy using make is something introduced as part of 0.6.0 IIRC and I don't think that it has a huge based of users

@zroubalik
Copy link
Member

Yes, so that's another incetive for you to complete ScalingSets ASAP :)) So let's try to merge this once ScalingSets are completed. I think the benefits are clera, let's use same naming for all methods etc.

As I mentioned, I would prefer if we use keda-http-add-on-... prefix instead of the keda-add-ons-http-... so it is aligned with the repo name and also the plural in add-ons doesn't much make sense.

I would not keep both names in the charts, they would become too complex and confusing imho.

@JorTurFer how does that sound to you?

@JorTurFer
Copy link
Member

Yes, so that's another incetive for you to complete ScalingSets ASAP :))

lol, no pressure xD

So let's try to merge this once ScalingSets are completed

My point is that ScalingSets feature has to be merged and released (so this PR can be merged for v0.10.0 or so)

As I mentioned, I would prefer if we use keda-http-add-on-... prefix instead of the keda-add-ons-http-... so it is aligned with the repo name and also the plural in add-ons doesn't much make sense.

I prefer this name too, I don't like the other naming at all, but 🤷 xD

@zroubalik
Copy link
Member

Yes, so that's another incetive for you to complete ScalingSets ASAP :))

lol, no pressure xD

Are you sure? 😄

@kahirokunn
Copy link
Contributor Author

Go Bold 👍

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.

None yet

5 participants