-
Notifications
You must be signed in to change notification settings - Fork 270
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
ignore ingresses without specified class #527
ignore ingresses without specified class #527
Conversation
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.
Hi, thanks for supporting this controller! See below a few changing suggestions.
@@ -36,6 +36,7 @@ The following command-line options are supported: | |||
| [`--verify-hostname`](#verify-hostname) | [true\|false] | `true` | | | |||
| [`--wait-before-shutdown`](#wait-before-shutdown) | seconds as integer | `0` | v0.8 | | |||
| [`--watch-namespace`](#watch-namespace) | namespace | all namespaces | | | |||
| [`--ignore-ingress-without-class`](#ignore-ingress-without-class)| [true\|false] | `false` | | |
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.
Use alphabetical order. Add also v0.10
in the Since column.
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.
made the changes, please verify
|
||
## --ignore-ingress-without-class | ||
|
||
Defines if the ingress without the ingress.class annotation will be considered or not. If `--ignore-ingress-without-class=true` then only the ingresses with the matching ingress.class annotation will be considered, ingresses with missing or different ingress.class annotation will not be considered. Default is false. |
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.
Use alphabetical order as well.
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.
made the changes, please verify
@@ -136,7 +137,8 @@ const IngressClassKey = "kubernetes.io/ingress.class" | |||
// IsValidClass ... | |||
func (ic *GenericController) IsValidClass(ing *extensions.Ingress) bool { | |||
ann, found := ing.Annotations[IngressClassKey] | |||
return !found || ann == ic.cfg.IngressClass | |||
|
|||
return (ic.cfg.IgnoreIngressWithoutClass && found && ann == ic.cfg.IngressClass) || (!ic.cfg.IgnoreIngressWithoutClass && (!found || ann == ic.cfg.IngressClass)) |
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.
Change to a simpler approach, eg (didn't test)
if !found {
return !ic.cfg.IgnoreIngressWithoutClass
}
return ann == ic.cfg.IngressClass
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.
made the changes, please verify
…simplified the if else logic
lgtm, thanks! Merging. |
Provide the flag to ignore ingresses without a specified class.
Fixes #522