search

banzaicloud / koperator

Oh no! Yet another Apache Kafka operator for Kubernetes
Apache License 2.0
783 stars 195 forks source link

Eliminate hardcoded values + move them at the beginning of file #1043

Closed szykes closed 1 year ago

szykes commented 1 year ago

Description

The goal of this PR to make visible the hardcoded values in the code.

Type of Change

Checklist

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

szykes commented 1 year ago

Question: is it intentional to remove the comments for the existing const variables? If so, why?

It was intentional. I think the comments were information duplications.

// DefaultKafkaImage is the default Kafka image used when users don't specify it in KafkaClusterSpec.ClusterImage DefaultKafkaImage = "ghcr.io/banzaicloud/kafka:2.13-3.4.1"

vs

// Kafka Cluster Spec ... defaultKafkaImage = "ghcr.io/banzaicloud/kafka:2.13-3.4.1"

panyuenlau commented 1 year ago

I think the comments were information duplications.

Well, I don't necessary agree this for all the const vars..., for example:

    // DefaultAnyCastPort kafka anycast port that can be used by clients for metadata queries
    DefaultAnyCastPort = 29092
    // DefaultIngressControllerTargetPort is the default container port for the ingress controller
    DefaultIngressControllerTargetPort = 29092

without the comments, I don't think these two vars are straightforward enough for others to understand what they are used for (assuming without looking into the implementation).

But for the rest, I am with you that the comments could be removed

szykes commented 1 year ago

The commenting thing is not an easy topic.

I have kept the first code comment of your comment. In addition, I have added the "chart path" for most of the defaults.