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

Fix hardcoded controller class #186

Merged
merged 1 commit into from
Sep 29, 2017
Merged

Conversation

mikhno-s
Copy link

@mikhno-s mikhno-s commented Sep 8, 2017

Add flag nginx-class, which can be set specific class for running multiple nginx ingress controllers.

@pleshakov
Copy link
Contributor

@SiriusRed thanks!

A couple of suggestions:

  • This PR breaks the current behavior, which is described here . Currently, Ingress resources with the annotation kubernetes.io/ingress.class: "nginx" or no annotation are handled by NGINX Ingress controller. It is better not to change this behavior. If you would like to make the Ingress controller process only Ingress resources with the annotation kubernetes.io/ingress.class: "nginx", I suggest to have an additional Boolean command-line flag, something like ingress-class-only.
  • It is better to rename the flag from nginx-class to ingress-class

@mikhno-s
Copy link
Author

Thanks for remark, mr. @pleshakov!

  • Renamed the flag nginx-class to ingress-class, it is more clearly.
  • Add flag 'use-ingress-class-only' for handled ingresses only with matching value of 'ingress-class'.

@pleshakov
Copy link
Contributor

Thanks @SiriusRed

I have a few additional suggestions:

  • Could you change VERSION in the Makefile from 1.0.1 to 1.0.0. We only change VERSION, when we make a new release?
  • Could you run all modified go files through go fmt tool to fix formatting?
  • Could you squash all your commits into one commit?

Also, there is a bug. When use-ingress-class-only option is enabled, if an Ingress resource doesn't have the ingress.class annotation, the Ingress controller processes this Ingress resource. It should not process such an Ingress resource, but only Ingress resources for the configured Ingress class.

@mikhno-s
Copy link
Author

mikhno-s commented Sep 25, 2017

Thanks @pleshakov for clearly instructions!
Done all of thing which you said. Added comment.

@pleshakov
Copy link
Contributor

@SiriusRed
Thanks. Works as expected now!

Could you make one last change - rename the variables useIngressClass to useIngressClassOnly? This will help make the code more readable.

@mikhno-s
Copy link
Author

@pleshakov
Done!

@pleshakov pleshakov merged commit a7d8e30 into nginxinc:master Sep 29, 2017
@pleshakov
Copy link
Contributor

@SiriusRed Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants