-
Notifications
You must be signed in to change notification settings - Fork 760
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
Add condition to disable nginx even if not exposing via ingress #1687
base: main
Are you sure you want to change the base?
Conversation
…e via ingress Signed-off-by: Raul Garcia Sanchez <info@raulgarcia.de>
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also need to update the README.
Signed-off-by: Raul Garcia Sanchez <info@raulgarcia.de>
Signed-off-by: Raul Garcia Sanchez <info@raulgarcia.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome
Signed-off-by: Raul Garcia Sanchez <info@raulgarcia.de>
Signed-off-by: Raul Garcia Sanchez <info@raulgarcia.de>
Signed-off-by: Raul Garcia Sanchez <info@raulgarcia.de>
Signed-off-by: Raul Garcia Sanchez <info@raulgarcia.de>
@@ -78,6 +80,7 @@ The following table lists the configurable parameters of the Harbor chart and th | |||
| Parameter | Description | Default | | |||
| -------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------- | | |||
| **Expose** | | | | |||
| `expose.enabled` | Set to false if no `ingress`, `clusterIP`, `nodePort` or `loadBalancer` should be created or you plan to expose Harbor in a way not offered by this chart. | `true` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any update here?
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. |
@MinerYang @wy65701436, can we get this thing merged in? What is topping us from doing so? |
Any update here? |
No description provided.