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 ports customisation options for ingress controller service. #479

Merged
merged 1 commit into from
Jan 16, 2019
Merged

Add ports customisation options for ingress controller service. #479

merged 1 commit into from
Jan 16, 2019

Conversation

sombralibre
Copy link
Contributor

Proposed changes

Add needed parameters to helm chart in orders to use custom options for the follow features:

  • Use custom NodePort numbers in the k8s service.
  • Enable/Disable options for http/https port service.
  • Set custom ports number for http/https port service.

Notes

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

Take into account the port spec in httpPort|httpsPort within values.yaml must match the container ports in the deployment|daemonset; another option would be add parametrized targetPort to the values.yaml and modify subsequents yaml files like controller-deployment.yaml and controller-daemonset.yaml.

Checklist

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

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

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

@Dean-Coakley Dean-Coakley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Just some small changes for consistency

deployments/helm-chart/README.md Outdated Show resolved Hide resolved
deployments/helm-chart/README.md Outdated Show resolved Hide resolved
deployments/helm-chart/README.md Outdated Show resolved Hide resolved
deployments/helm-chart/README.md Outdated Show resolved Hide resolved
deployments/helm-chart/README.md Outdated Show resolved Hide resolved
deployments/helm-chart/README.md Outdated Show resolved Hide resolved
deployments/helm-chart/templates/controller-service.yaml Outdated Show resolved Hide resolved
deployments/helm-chart/templates/controller-service.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@Dean-Coakley Dean-Coakley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes. Looks like you missed a few small things. Happy to merge once these are resolved.

Could you possibly squash your commits into one commit?

deployments/helm-chart/README.md Outdated Show resolved Hide resolved
deployments/helm-chart/README.md Outdated Show resolved Hide resolved
deployments/helm-chart/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@Dean-Coakley Dean-Coakley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Dean-Coakley Dean-Coakley merged commit 26bc64b into nginxinc:master Jan 16, 2019
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