Skip to content

Conversation

@sajal
Copy link

@sajal sajal commented Aug 16, 2017

Image built from my fork : turbobytes/nginx-ingress:0.9.0

This adds a configmap key lb-method and annotation nginx.org/lb-method as suggested by @pleshakov in #94 .

If this setting is not blank, then the value is pasted into the upstream block in the nginx config. This is configurable per Ingress, and not per service.

@pleshakov
Copy link
Contributor

@sajal
thanks for the PR!

Here is a couple of suggestions:

  1. In the documentation, could you change the default value from N/A to "" and also add the following statement after "Sets the load balancing method.": The "" specifies the round-robin method.
  2. I think it is better to pass the ingCfg to the createUpstream and extract the method from it, rather than from ingEx as in https://github.com/turbobytes/kubernetes-ingress/blob/e4a43d4e700969fd9d71a90fd879350ba2c3f486/nginx-controller/nginx/configurator.go#L449 Thus, we don't need to have LBMethod string as part of IngressEx. It is sufficient to have LBMethod only in Config and Upstream

@sajal
Copy link
Author

sajal commented Aug 16, 2017

@pleshakov Good ideas. I will look into them now.

@sajal
Copy link
Author

sajal commented Aug 16, 2017

@pleshakov Now im passing LBMethod as argument instead of using IngressEx

@pleshakov
Copy link
Contributor

@sajal great! thx

I have two more suggestions:

Could you move this logic https://github.com/turbobytes/kubernetes-ingress/blob/b819d7e0acdb119541b7d91228167af99c6e5b5f/nginx-controller/nginx/configurator.go#L86-L88 to the createConfig method for consistency. This way all annotation parsing happens only in one place.

Could you also squash you commits into a single one?

@sajal
Copy link
Author

sajal commented Aug 17, 2017

Ah so I will overwrite the annotation value id present at ingCfg in createConfig.

@sajal
Copy link
Author

sajal commented Aug 17, 2017

@pleshakov : moved annotation checking into createConfig and squash'd

@pleshakov pleshakov merged commit 8117c06 into nginx:master Aug 18, 2017
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