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

Provide consistent naming for configuration #712

Closed
tomkerkhove opened this issue Mar 31, 2020 · 9 comments
Closed

Provide consistent naming for configuration #712

tomkerkhove opened this issue Mar 31, 2020 · 9 comments
Milestone

Comments

@tomkerkhove
Copy link
Member

Provide consistent naming for configuration across scalers.

As per #694 we've agreed that configuration should be aligned and use the same name & patterns for configuration.

Things that came up:

  • Most scalers provide a field for a full address, we should standardize on:
    • connectionString for, well, connection strings :)
    • connection for {host}:{port} connections to servers et all
  • Scalers connecting to a host often need a port. Some scalers provide flexibility to specify host and port seperately as well, we should provide this for all scalers where applicable so that
    • connection is used when available, otherwise it composes it based on host & port
    • If none of them are provided (or only host and not port or other way around), and exception should be thrown
@tomkerkhove
Copy link
Member Author

Thoughts? Anything that was missed?

@inuyasha82
Copy link
Contributor

inuyasha82 commented Apr 2, 2020

@tomkerkhove There is only one question: if this new naming convention will be adopted, it has to be retrocompatible?

Because for example if we are going to rename address in connection and similr, people already using that scalers if they will update will result in their environment screwed up.

Or maybe start with a deprecation of old parameters (printing a warning in the logs) for a while before removing the old stuff?

Or just document all the changes, and users has to read the changelog before updating (since it is a major version upgrade it could be kinda fine?)

@tomkerkhove
Copy link
Member Author

We'll break it since it will only be added to 2.0 - We are not sure yet which scalers it impacts and if there will be a new minor version; but when we ship this migration guidance will be provided.

Deprecation notices without clear path on what to use instead is not ideal; certainly when the new parts are not there yet.

@TsuyoshiUshio
Copy link
Contributor

Hi @tomkerkhove

I believe establish the standard before v2 GA sounds awesome.
This is just my personal opinion, however, I think the each configuration should follow the each SDK's terminologies. Maybe we need to discuss it with others.

The reason is :

  • Customers might not interested the other scalers they use
  • Customers is familiar with the terminology of the each resources, eg, Azure Queue, Rabbit MQ, EventHubs and so on. They are reading these documentation
  • There are several authentication methods, the naming is representing the differences between them

I have other things for the standard. It is also just an idea.

  • Should we use the direct parameter, or use environment variables or both?
  • How we can do local debugging if the resources are on k8s cluster.

@tomkerkhove
Copy link
Member Author

We are actually doing the exercise in this issue with a Google doc open: #753 (comment)

Feel free to go through it and add comments!

Customers might not interested the other scalers they use

That's true, but what about those who are interested? No point in having one scaller be called host and the other connection for example.

How we can do local debugging if the resources are on k8s cluster.

Not sure how this relates, do you mean local KEDA dev?

@zroubalik
Copy link
Member

my 2 cents:

I think that we should keep the convention/naming that is usual for each individual scaler, (eg. Kafka is using boostrapServers and not host or connection, b/c that's something that typical for Kafka)

Those scalers, where aren't that strict conventions we should do some unification (eg. host, host name, etc) if it is needed.

As it is discussed on #753 (comment), I think that we should allow users to use direct parametr or reference for the properties where it does make sense, eg. connection and connectionRef where connection has preference if user specify both. Currently there is this ambiguity in some scalers (Redis, where users expect direct parameter, but reference is required instead. So we should unify this across scalers and provide both options.

@TsuyoshiUshio
Copy link
Contributor

How we can do local debugging if the resources are on k8s cluster.

Not sure how this relates, do you mean local KEDA dev?

It might not need to consider this discussion. :( sorry about that.

I mean Yes. For example, Let's imagine we debug locally with RabbitMQ deployed on a k8s cluster. We need to deploy Job or Deployment on the cluster. The pod should refer the endpoint of the RabbitMQ, it should be the value that is solved inside of the k8s cluster.
However, if you want to debug locally on your machine with the operator-sdk, the server running on your machine. We can refer the RabbitMQ endpoint from your PC by proxy, however, the endpoint will be different one. e.g. localhost:5672. For the current implementation, it refers to configuration of the host so that we need to modify the code to debug it.

So I used to send pull request to rabbit mq scaler like this, however, it must be much better to solve it via controller rather than individual scalers. That is why I close the PR.
#976 However, the configuration/logic might not be included on the scalers side so never mind.

@zroubalik
Copy link
Member

@tomkerkhove can we close this one?

@tomkerkhove
Copy link
Member Author

Certainly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants