Add condition to disable nginx even if not exposing via ingress

[rgarcia89]

No description provided.

[Vad1mo]

This is the same as #1073

we should unite those two PRs.

@zyyw @ninjadq @wy65701436, we need such a PR more than ever more and more people are using different Ingresses nowadays, especially with GatewayAPI. Harbor can not support all the different options, so IMO we should add the option to not render the Ingress at all as we can not cover all the edge cases.

[Vad1mo]

we also need to update the README.

[Vad1mo]

awesome

[wy65701436]

Instead of adding a new item, I prefer to include additional Enums such as "None" for the expose type. This approach will not overly complicate the code.

And when set the value of expose type to none, we do nothing for ingress and nginx.

[rgarcia89]

In my humble opinion, I believe that an enable setter aligns better with the current chart structure.
internalTLS, persistence, trivy, metrics, and trace each have one.
So, essentially, all dispensable components have one. Why deviate from this approach here now?

[Vad1mo]

I agree enable = true/false is common practice in Helm Chart development, we also use it in that chart. While it seem to be more verbose in code it is much easier and straightforward to understand for users what is going on and what is going to happen

[MinerYang]

enabled more like a two sides thing, like whether using internalTLS or not, there's no third choice. But for expose, we have so many choices already, and made our control flow sometimes redundant with both type and enabled . We would like to make our templates as simple as possible and in the meantime fulfill the requirements.
I will take some time to maybe have an overall adjustment for this part, and please @rgarcia89 help to review on it if your are willing to.

[Vad1mo]

I don't understnad why you think this is to complex logic.

{{- if and (ne .Values.expose.type "ingress") .Values.nginx.enabled }}

• This is common practice, in writing helm charts, every best practice blog mentions that practive.
• it is much easier to test in the subsequent logic .Values.nginx.enabled instead of .if Values.expose.type == None
• It doesn't follow that https://en.wikipedia.org/wiki/Single_responsibility_principle. Values.expose.type would have two different meaning and depending on the value cause implicitly different behaivor, which is difficult to reason about in the long run. especially for the average Chart Developer. (I never knew I would say that in terms of YAML, but its true)

[rgarcia89]

I agree with @Vad1mo on that. I don't see why adding an enable switch should increase the complexity of the template. From my understanding, this is following a very simple approach and is not creating a misleading situation, which could be created by a type: none expose.

[MinerYang]

I agree it is not complex, and my first thoughts was could we make it more simple when we only use expose.type as the only entry-point instead of combine with expose.type and nginx.enabled for the proxy usage.
maybe have something like this in values.yaml

expose:
    ## enum type Nginx, Ingress, ClusterIP,  NodePort, Loadbalancer, Others
    type:
    nginx:
        image:
        ...
    ingress:
       hosts:
       ...
    clusterIP:
        name:
        ...
    loadBalancer:
        name:
        ...
    nodeport:
        name:
        ...

While the templates would be something like this

{{- if Values.expose.type == nginx }}

Sure I will take your comments into consideration, And please point me out if I have something understood wrong. Thx~

[rgarcia89]

Any update here?

[github-actions]

This PR is being marked stale due to a period of inactivty. If this PR is still relevant, please comment or remove the stale label. Otherwise, this PR will close in 30 days.

[Vad1mo]

@MinerYang @wy65701436, can we get this thing merged in? What is topping us from doing so?