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

Document (and add!) new flags for 1.0.0 #871

Merged
merged 3 commits into from
Sep 29, 2020
Merged

Document (and add!) new flags for 1.0.0 #871

merged 3 commits into from
Sep 29, 2020

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Sep 22, 2020

What this PR does / why we need it:
Adds documentation for new Ingress API version and class matching flags.
Adds several flags that I forgot to add everywhere earlier 🤦

Special notes for your reviewer:
Whoops, forgot to touch all the flag instances in flags.go in #843 🤦

@rainest rainest requested a review from hbagdi September 22, 2020 20:16
@rainest rainest changed the base branch from next to main September 22, 2020 23:55
@rainest rainest changed the base branch from main to next September 22, 2020 23:55
@rainest
Copy link
Contributor Author

rainest commented Sep 23, 2020

Was originally going to rebase this to main or open up a separate PR with only the commit that adds the defaults, but on further review, while this is still a goof, it shouldn't matter. Everything in question defaults to false, which is effectively what viper defaults to anyway when it can't find a boolean flag. We already get these in https://github.com/Kong/kubernetes-ingress-controller/blob/0.10.0/cli/ingress-controller/flags.go#L346, so...

https://github.com/spf13/viper/blob/v1.7.1/viper.go#L797-L799

https://github.com/spf13/cast/blob/v1.3.1/cast.go#L12-L15
https://github.com/spf13/cast/blob/v1.3.1/caste.go#L80-L81

No one was ever the wiser!

So still 🤦‍♀️ but doesn't warrant a patch release to fix the issue in 0.10.x.

@@ -59,6 +62,9 @@ Following table describes all the flags that are available:
| --profiling |`boolean` | `true` | Enable profiling via web interface `host:port/debug/pprof/`. |
| --publish-service |`string` | none | The namespaces and name of the Kubernetes Service fronting Kong Ingress Controller in the form of namespace/name. The controller will set the status of the Ingress resouces to match the endpoints of this service. In reference deployments, this is kong/kong-proxy.|
| --publish-status-address |`string` | none | User customized address to be set in the status of ingress resources. The controller will set the endpoint records on the ingress using this address.|
| --skip-classless-ingress-v1beta1 |`boolean` | `false` | Toggles whether the controller processes `extensions/v1beta1` and `networking/v1beta1` Ingress resources that have no `kubernetes.io/ingress.class` annotation.|
Copy link
Member

Choose a reason for hiding this comment

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

This should be --process-classless-ingress-v1

Copy link
Contributor

@mflendrich mflendrich left a comment

Choose a reason for hiding this comment

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

LGTM.

This PR wouldn't have been needed in the first place (and discrepancies wouldn't have had a chance to occur) if we had implemented the improvements below:

@@ -37,6 +37,9 @@ Following table describes all the flags that are available:
| --alsologtostderr |`boolean` | `false` | Logs are written to standard error as well as to files.|
| --anonymous-reports |`string` | `true` | Send anonymized usage data to help improve Kong.|
| --apiserver-host |`string` | none | The address of the Kubernetes Apiserver to connect to in the format of protocol://address:port, e.g., "http://localhost:8080. If not specified, the assumption is that the binary runs inside a Kubernetes cluster and local discovery is attempted.|
| --disable-ingress-extensionsv1beta1 |`boolean` | `false` | Disable processing Ingress resources with apiVersion `extensions/v1beta1`.|
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking: I don't see a point in maintaining this table separately from pflag flag definitions in the code (and the --help resulting from there).

By maintaining it, we're asking for extra maintenance burden, and discrepancy bugs.

Tracked (and the necessity of this table to be discussed with @rainest @hbagdi) in #880

@hbagdi hbagdi merged commit 1114a48 into next Sep 29, 2020
@hbagdi hbagdi deleted the doc/new-flags branch September 29, 2020 16:19
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.

3 participants