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

Add custom NodePort to helm controller-service. #477

Closed
wants to merge 4 commits into from
Closed

Add custom NodePort to helm controller-service. #477

wants to merge 4 commits into from

Conversation

sombralibre
Copy link
Contributor

Proposed changes

Add two configuration parameters to helm chart controller service, in order to use custom ports number when using NodePort.

These are the new parameters:
controller.service.NodePort.Http
controller.service.NodePort.Https

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • [ X] I have read the CONTRIBUTING doc
  • [ X] I have added tests that prove my fix is effective or that my feature works
  • [ X] I have checked that all unit tests pass after adding my changes
  • [ X] I have updated necessary documentation
  • [ X] I have rebased my branch onto master
  • [ X] I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

@pleshakov pleshakov added the enhancement Pull requests for new features/feature enhancements label Jan 14, 2019
@Dean-Coakley
Copy link
Contributor

Hi @sombralibre

Thanks for the PR!

Can you give the version number in Chart.yaml a minor bump?

@Dean-Coakley
Copy link
Contributor

Hi @sombralibre,

Apologies for not mentioning this in my initial review.

I think that the nodePort customisation should be implemented slightly differently, taking into consideration the other useful customisation options for http and https ports.

A possible solution could look like:

values.yaml

service:
    httpPort:
        enable: true
        port: 80
        nodePort: ""

    httpsPort:
        enable: true
        port: 443
        nodePort: ""

controller-service.yaml

ports:
{{- if .Values.controller.service.httpPort.enable }}
  - port: {{ .Values.controller.service.httpPort.port }}
    targetPort: 80
    . . .
  {{- end }}
  {{- if .Values.controller.service.httpsPort.enable }}
  - port: {{ .Values.controller.service.httpsPort.port }}
    targetPort: 443
    . . .
{{- end }}

What do you think?

I do realise that this solution requires some major changes to this PR. Therefore I’d be happy to implement them. However, if you still would like to update this PR, please let me know.

@sombralibre
Copy link
Contributor Author

Looks cool,

I want to perform these changes because they will be needed to use this ingress-controller into my company. So can I push the changes to this PR? or will be better to open a new one?

@pleshakov
Copy link
Contributor

@sombralibre
great! Yes, considering the changes, it'd be better to implement them in a new PR.

@sombralibre
Copy link
Contributor Author

I've pushed the changes to this new PR #479

@Dean-Coakley
Copy link
Contributor

Closing in favour of #479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants