http-add-on icon indicating copy to clipboard operation
http-add-on copied to clipboard

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

Open kahirokunn opened this issue 1 year ago • 18 comments

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

  • [x] Commits are signed with Developer Certificate of Origin (DCO)
  • [ ] Changelog has been updated and is aligned with our changelog requirements
  • [x] Any necessary documentation is added, such as:

Fixes #

kahirokunn avatar May 21 '24 05:05 kahirokunn

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

JorTurFer avatar May 21 '24 06:05 JorTurFer

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

kahirokunn avatar May 21 '24 13:05 kahirokunn

Should we abandon the PR then?

tomkerkhove avatar May 23 '24 12:05 tomkerkhove

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.

kahirokunn avatar May 23 '24 23:05 kahirokunn

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.

zroubalik avatar Jun 06 '24 14:06 zroubalik

@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.

kahirokunn avatar Jun 10 '24 00:06 kahirokunn

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

tbh, I also got a bit confused initially by the discrepancy in naming. I took 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 :)

wozniakjan avatar Jun 10 '24 12:06 wozniakjan

Thank you LGTM!

kahirokunn avatar Jun 10 '24 12:06 kahirokunn

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

JorTurFer avatar Jun 24 '24 11:06 JorTurFer

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.

zroubalik avatar Jun 24 '24 12:06 zroubalik

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

JorTurFer avatar Jun 24 '24 13:06 JorTurFer

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?

zroubalik avatar Jun 24 '24 13:06 zroubalik

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

JorTurFer avatar Jun 24 '24 13:06 JorTurFer

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

lol, no pressure xD

Are you sure? 😄

zroubalik avatar Jun 24 '24 14:06 zroubalik

Go Bold 👍

kahirokunn avatar Jul 04 '24 07:07 kahirokunn

Hi 🤚

kahirokunn avatar Oct 10 '24 00:10 kahirokunn

Hi 🤚

Hi! apologies for the slow responses, given the chart might be actually more frequently used and I am not aware of a way to easily and without breaking changes rename the chart, I'm wondering if the rename shouldn't be the other way around. When I look at the diff it performs keda-add-ons-http-interceptor-proxy -> keda-http-add-on-interceptor-proxy (despite the title in the PR)

wozniakjan avatar Oct 21 '24 11:10 wozniakjan

Hi 🤚

Hi! apologies for the slow responses, given the chart might be actually more frequently used and I am not aware of a way to easily and without breaking changes rename the chart, I'm wondering if the rename shouldn't be the other way around. When I look at the diff it performs keda-add-ons-http-interceptor-proxy -> keda-http-add-on-interceptor-proxy (despite the title in the PR)

Not important rant - I don't personally like the plural version (add-ons) :))

zroubalik avatar Oct 21 '24 17:10 zroubalik

@wozniakjan Sorry, I fixed it.

kahirokunn avatar Oct 31 '24 07:10 kahirokunn

How about now?

kahirokunn avatar Dec 09 '24 03:12 kahirokunn

Bump up 👀

kahirokunn avatar Jan 14 '25 00:01 kahirokunn

I have authored tutorials and documentation for KEDA on multiple occasions, and in doing so, I have found the inconsistencies in its name to be quite troublesome. I fear that allowing this situation to continue will only serve to hinder KEDA’s growth. Above all, I truly hope for a future in which this issue no longer exists, and I am committed to making that vision a reality.

kahirokunn avatar Jan 14 '25 00:01 kahirokunn

@kahirokunn thank you very much for your efforts, it's greatly appreciated. After some user discovery, I came to the conclusion that it would be now better to rename the other way and follow the convention set up by the helm chart. It looks like majority of the users install this through the chart and changing the service name would require manual effort to rewrite all Ingress objects for them.

Unless @JorTurFer and @zroubalik object, I would like to propose going this way

keda-http-add-on-interceptor-proxy -> keda-add-ons-http-interceptor-proxy

wozniakjan avatar Jan 31 '25 15:01 wozniakjan

@wozniakjan I agree with this proposal. While naming conventions are certainly important, consistency is paramount. When there is consistency, confusion is eliminated. If @JorTurFer and @zroubalik are in agreement, I am willing to work on creating a new PR. I would greatly appreciate hearing both of your thoughts on this matter.

kahirokunn avatar Feb 01 '25 00:02 kahirokunn

Yeah, agreed with @wozniakjan

JorTurFer avatar Feb 10 '25 22:02 JorTurFer

Thank you. I have standardized all instances of keda-http-add-on-interceptor-proxy to keda-add-ons-http-interceptor-proxy. I would appreciate your review. Thx 🙏

kahirokunn avatar Feb 13 '25 23:02 kahirokunn

Thank you 🙏 I also dealt with the docs you pointed out. I think I've dealt with everything now, but what do you think?

kahirokunn avatar Feb 19 '25 01:02 kahirokunn