Improve Project Layout and Refactor Controller Package #357

peterkellydev · 2018-09-05T09:43:49Z

Proposed changes

After the project restructure PR (this branch is branches from that one), this PR then pulls apart some of the complexity in the load balancer controller. Extracts handlers and builds them separately.

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

Following this common approach https://github.com/golang-standards/project-layout

isserrano · 2018-09-05T10:00:05Z

If go test ./... fixed, everything else LGTM

isserrano

Waiting to fix CI stages

isserrano · 2018-09-05T11:01:43Z

CI fixed, build is fine

isaachawley

👍

pleshakov

Please see my comments. Additionally,

Also, please update links to nginx-controller folder in the following files:

examples/customization/README.md in log-format and stream-log-format cm keys
build.README.md. , ... in of your license are located in the nginx-controller folder: The keys must be placed in kubernetes-ingress now.

The helm-chart related files must be updated according with the new project layout:

Chart.yaml: Update https://github.com/nginxinc/kubernetes-ingress/tree/master/helm-chart

The install folder is not completely removed.

The code refactoring brought a lot of public uncommented method, so we now get linter warnings.

pleshakov · 2018-09-05T11:42:25Z

.gitignore

 osx-nginx-plus-ingress
 nginx-plus-ingress
-nginx-controller/nginx-controller
+cmd/nginx-ic/nginx-ic


this line is not needed

it is needed as it is the binary produced from go build, but it should be nginx-ingress

it seems that the following lines cover that:

nginx-ingress !nginx-ingress/

pleshakov · 2018-09-05T11:45:05Z

CONTRIBUTING.md

-* The main code resides under `/nginx-controller`
-* The project dependencies reside in the `/vendor`. We use  [dep](https://github.com/golang/dep) for managing dependencies.
+* The project follows a standard Go project layout
+    * The main code is found at `cmd/nginx-ic/`


the actual folder is cmd/nginx-ingress/

pleshakov · 2018-09-05T11:58:49Z

Makefile

@@ -46,3 +49,4 @@ endif

 clean:
 	rm -f nginx-ingress
+	rm $(DOCKERFILE)


When make is called with DOCKERFILE=DockerfileForPlus, DockerfileForPlus is copied to /. As a result, make clean must also be called with DOCKERFILE=DockerfileForPlus. Perhaps we can copy DockerfileForPlus or any other as Dockerfile?

pleshakov · 2018-09-05T12:04:04Z

README.md

@@ -71,7 +71,7 @@ NGINX Plus provides you with [advanced statistics](https://www.nginx.com/product
 * **JWTs** NGINX Plus can validate JSON Web Tokens (JWTs), providing a flexible authentication mechanism.
 * **Support** Support from NGINX Inc is available for NGINX Plus Ingress controller.

-**Note**: Deployment of the Ingress controller for NGINX Plus requires you to do one extra step: build your own [Docker image](nginx-controller) using the certificate and key for your subscription.
+**Note**: Deployment of the Ingress controller for NGINX Plus requires you to do one extra step: build your own [Docker image](build) using the certificate and key for your subscription.


Please also update 2 links for the edge version for build your own image in the NGINX Ingress Controller Releases section

links to the Helm chart as well in the same section

and a link to manifests in the same section

pleshakov · 2018-09-05T12:05:28Z

build/Dockerfile

@@ -6,7 +6,7 @@ RUN ln -sf /proc/1/fd/1 /var/log/nginx/access.log \
 	&& ln -sf /proc/1/fd/1 /var/log/nginx/stream-access.log \
 	&& ln -sf /proc/1/fd/2 /var/log/nginx/error.log

-COPY nginx-ingress nginx/templates/nginx.ingress.tmpl nginx/templates/nginx.tmpl /
+COPY nginx-ingress internal/nginx/templates/nginx.ingress.tmpl internal/nginx/templates/nginx.tmpl /


DockerfileForAlpine and DockerfileForPlus require a similar update

pleshakov · 2018-09-05T12:08:41Z

build/README.md

    ```
    $ git clone https://github.com/nginxinc/kubernetes-ingress/
-    $ cd kubernetes-ingress/nginx-controller
    ```

 1. If you're using a stable release, check out the corresponding tag. For release 1.3.0, run:


I suggest removing this paragraph completely, cause it will check out the old structure

pleshakov · 2018-09-05T12:10:48Z

Makefile

@@ -4,22 +4,24 @@ VERSION = edge
 TAG = $(VERSION)
 PREFIX = nginx/nginx-ingress

-DOCKER_RUN = docker run --rm -v $(shell pwd)/../:/go/src/github.com/nginxinc/kubernetes-ingress -w /go/src/github.com/nginxinc/kubernetes-ingress/nginx-controller/
+DOCKER_RUN = docker run --rm -v $(shell pwd):/go/src/github.com/nginxinc/kubernetes-ingress


will DOCKER_TEST_RUN make more sense as a name?

I am not going to change this in this PR

pleshakov · 2018-09-05T12:28:25Z

deployments/helm-chart/README.md

@@ -10,7 +10,7 @@ This chart deploys the NGINX Ingress controller in your Kubernetes cluster.
  - Helm 2.8.x+.
  - Git.
  - If you’d like to use NGINX Plus:
-    - Build an Ingress controller image with NGINX Plus and push it to your private registry by following the instructions from [here](../nginx-controller/README.md).
+    - Build an Ingress controller image with NGINX Plus and push it to your private registry by following the instructions from [here](../../build/README.md).
    - Update the `controller.image.repository` field of the `values-plus.yaml` accordingly.


Please also remove If you're using a stable release, check out the corresponding tag. For release 1.3.0 paragraph.

Also, update Change your working directory to /helm-chart, cd kubernetes-ingress/helm-chart x2

pleshakov · 2018-09-05T12:37:20Z

docs/installation.md

@@ -5,7 +5,7 @@
 Make sure you have access to the Ingress controller image:

 * For NGINX Ingress controller, use the image `nginx/nginx-ingress` from [DockerHub](https://hub.docker.com/r/nginx/nginx-ingress/).
-* For NGINX Plus Ingress controller, build your own image and push it to your private Docker registry by following the instructions from [here](../nginx-controller).
+* For NGINX Plus Ingress controller, build your own image and push it to your private Docker registry by following the instructions from [here](../build/README.md).

 The installation manifests are located in the [install](../install) folder. In the steps below we assume that you will be running the commands from that folder.


please rename install to deployments

- Addresses all `golint` warnings including ones not caused by this PR - Moves install files to correct place - Updates docs with old references before structure - Fixes broken links (not caused by this PR) - Updates COPY command in other Dockerfiles

Peter Kelly added 2 commits September 5, 2018 09:59

Refactor package layout to be more standard

bc78a8b

Following this common approach https://github.com/golang-standards/project-layout

Update gitignore

ee6bf87

peterkellydev requested review from isaachawley, pleshakov and isserrano September 5, 2018 09:43

Peter Kelly added 4 commits September 5, 2018 11:07

Fix broken test from not moving test file for main

3a8b9fd

More refactoring, mainly tidy up of controller

5e66437

Extract handlers, reduce complexity of creating new Controller.

961e591

Fix failing tests from renaming

39b1338

peterkellydev force-pushed the controller-refactor branch from c5a1388 to 39b1338 Compare September 5, 2018 10:15

Change build location when not building in container

bc7eebd

isserrano approved these changes Sep 5, 2018

View reviewed changes

isaachawley approved these changes Sep 5, 2018

View reviewed changes

pleshakov suggested changes Sep 5, 2018

View reviewed changes

peterkellydev force-pushed the controller-refactor branch from c44de2d to f12960e Compare September 6, 2018 13:37

pleshakov changed the title ~~Controller refactor~~ Improve Project Layout and Refactor Controller Package Sep 6, 2018

Peter Kelly added 2 commits September 6, 2018 16:05

Use Dockerfile as copy target

82a75da

Use -f sp make clean will not error if file not present

2f39310

pleshakov approved these changes Sep 6, 2018

View reviewed changes

pleshakov added enhancement Pull requests for new features/feature enhancements change Pull requests that introduce a change labels Sep 6, 2018

peterkellydev merged commit cca1389 into master Sep 6, 2018

peterkellydev deleted the controller-refactor branch September 6, 2018 15:40

ydcool mentioned this pull request Nov 25, 2019

fix broken links kubernetes-retired/kube-aws#1781

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Project Layout and Refactor Controller Package #357

Improve Project Layout and Refactor Controller Package #357

peterkellydev commented Sep 5, 2018

isserrano commented Sep 5, 2018 •

edited

Loading

isserrano left a comment

isserrano commented Sep 5, 2018

isaachawley left a comment

pleshakov left a comment

pleshakov Sep 5, 2018

peterkellydev Sep 6, 2018

pleshakov Sep 6, 2018

pleshakov Sep 5, 2018

pleshakov Sep 5, 2018

pleshakov Sep 5, 2018

pleshakov Sep 5, 2018

pleshakov Sep 5, 2018

pleshakov Sep 5, 2018

pleshakov Sep 5, 2018

pleshakov Sep 5, 2018

peterkellydev Sep 6, 2018

pleshakov Sep 5, 2018

pleshakov Sep 5, 2018

Improve Project Layout and Refactor Controller Package #357

Improve Project Layout and Refactor Controller Package #357

Conversation

peterkellydev commented Sep 5, 2018

Proposed changes

Checklist

isserrano commented Sep 5, 2018 • edited Loading

isserrano left a comment

Choose a reason for hiding this comment

isserrano commented Sep 5, 2018

isaachawley left a comment

Choose a reason for hiding this comment

pleshakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isserrano commented Sep 5, 2018 •

edited

Loading