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

Simplify installation and e2e manifests #2515

Merged
merged 1 commit into from
May 17, 2018

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented May 15, 2018

Also, use RBAC by default.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 15, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 15, 2018
@aledbf aledbf force-pushed the simplify-installation branch from e80e893 to ab14771 Compare May 15, 2018 22:52
@aledbf
Copy link
Member Author

aledbf commented May 15, 2018

This can be merged after we release 0.15

@aledbf
Copy link
Member Author

aledbf commented May 15, 2018

ping @zrdaley @diazjf

@aledbf aledbf force-pushed the simplify-installation branch 2 times, most recently from c9e21e0 to 15b70bc Compare May 15, 2018 23:12
Copy link

@diazjf diazjf left a comment

Choose a reason for hiding this comment

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

Hey @aledbf thanks for the PR, makes it a lot easier to follow. I just have a few thoughts.

@@ -101,25 +58,8 @@ For development:
$ minikube addons disable ingress
Copy link

Choose a reason for hiding this comment

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

Add minikube addons disable addon-manager to this.

Copy link

Choose a reason for hiding this comment

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

Maybe we should just have everything enabled and then just edit the deployment?

```

7. Confirm the `nginx-ingress-controller` deployment exists:
2. Execute `make dev-env`
Copy link

Choose a reason for hiding this comment

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

Currently we perform the following:

kubectl set image \
    deployments \
    --namespace ingress-nginx \
    --selector app=ingress-nginx \
    nginx-ingress-controller=${REGISTRY}/nginx-ingress-controller:${TAG}

but in the future minikube will already have this deployment in the kube-system namespace and once that happens we can just edit the deployment on the kube-system namespace instead of creating a new one.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to assume minikube is only way to run tests

apiVersion: v1
kind: Namespace
metadata:
name: ingress-nginx
Copy link

Choose a reason for hiding this comment

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

Should we just use the kube-system namespace that minikube already provides.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is to use the manifest we provide to the users in this repository and also be able to run e2e tests in any kubernetes cluster

@@ -26,6 +26,7 @@ spec:
- --configmap=$(POD_NAMESPACE)/nginx-configuration
- --tcp-services-configmap=$(POD_NAMESPACE)/tcp-services
- --udp-services-configmap=$(POD_NAMESPACE)/udp-services
- --publish-service=$(POD_NAMESPACE)/ingress-nginx
Copy link

Choose a reason for hiding this comment

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

Just curious, whats this for?

Copy link
Member Author

Choose a reason for hiding this comment

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

The flag --publish-service is used to extract information about the Service fronting the ingress controllers. The controller will set the endpoint records on the ingress objects to reflect those on the service. If you don't use this flag the controller uses the IP address of the node

@aledbf aledbf force-pushed the simplify-installation branch from 15b70bc to aaa3820 Compare May 16, 2018 02:35
@aledbf
Copy link
Member Author

aledbf commented May 16, 2018

@diazjf also, at some point a migration to prow.k8s.io could be requested and that do not runs minikube.

@diazjf
Copy link

diazjf commented May 16, 2018

@aledbf makes sense to me. The user shouldn't be forced to minikube for development and testing. I mean in the case where make dev-env which relies on minikube, that we should make sure that all we do is simply edit the existing deploy in the kube-system namespace.

@aledbf aledbf force-pushed the simplify-installation branch 2 times, most recently from ed8d7bc to 2a34091 Compare May 16, 2018 19:02
@aledbf aledbf changed the title WIP: Simplify installation and e2e manifests Simplify installation and e2e manifests May 16, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 16, 2018
@@ -5,3 +5,5 @@ metadata:
namespace: ingress-nginx
labels:
app: ingress-nginx

---
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the YAML spec, this marks the beginning of a document, not the end.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@aledbf aledbf force-pushed the simplify-installation branch from 2a34091 to 4f4b0c5 Compare May 17, 2018 12:46
@aledbf aledbf force-pushed the simplify-installation branch from 4f4b0c5 to 7b29a0c Compare May 17, 2018 12:51
@codecov-io
Copy link

codecov-io commented May 17, 2018

Codecov Report

Merging #2515 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2515   +/-   ##
=======================================
  Coverage   40.73%   40.73%           
=======================================
  Files          73       73           
  Lines        5015     5015           
=======================================
  Hits         2043     2043           
  Misses       2700     2700           
  Partials      272      272

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ef8a0a...7b29a0c. Read the comment docs.

@aledbf aledbf merged commit f92f5f8 into kubernetes:master May 17, 2018
@aledbf aledbf deleted the simplify-installation branch May 25, 2018 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants