Create all-in-one file to deploy kube-app-manager #179

ant31 · 2020-02-24T14:26:05Z

related to #166

Images are pushed to quay.io/kubernetes-sigs/kube-app-manager
resources are generated to the "deploy/" directory
grouped webhook configuration all together in the config directory

barney-s · 2020-02-25T19:41:39Z

needs rebase

deploy/kube-app-manager-aio.yaml

Makefile

barney-s · 2020-02-27T07:22:54Z

Makefile

 # Deploy controller in the configured Kubernetes cluster in ~/.kube/config
 .PHONY: deploy
-deploy: $(TOOLBIN)/kustomize
+deploy: $(TOOLBIN)/kustomize update-version


use generate-resource as a make dependency

barney-s · 2020-02-27T07:23:18Z

Makefile

-deploy: $(TOOLBIN)/kustomize
+deploy: $(TOOLBIN)/kustomize update-version
 	cd config/kube-app-manager && $(TOOLBIN)/kustomize edit set image controller=$(CONTROLLER_IMG)
 	$(TOOLBIN)/kustomize build config/default | $(TOOLBIN)/kubectl apply -f -


use kubectl apply -f deploy/kube-app-manager...

barney-s · 2020-02-27T07:47:48Z

Makefile


+.PHONY: update-version
+update-version:
+	sed -i ''  "s|app.kubernetes.io/version: dev|app.kubernetes.io/version: $(VERSION)|g" config/default/kustomization.yaml


this seems like an anti-pattern. and not sure if there is a better way.

kubernetes-sigs/kustomize#388
@Liujingfang1 FYI

A better way of doing this is to create a new kustomization target for that version. For example, use config/v1.0.0 directory for the version v1.0.0.

Thanks for your recommendation @Liujingfang1

We can try this way(new kustomization target) as suggested, and discuss later if it becomes too much work

barney-s · 2020-02-27T07:49:57Z

config/kube-app-manager/kustomization.yaml

 resources:
 - manager.yaml
+
+images:


this may not be needed since we use kustomize set image in makefile

config/kube-app-manager/manager.yaml

barney-s · 2020-02-27T07:53:01Z

config/webhook/manager_auth_proxy_patch.yaml

+kind: Deployment
+metadata:
+  name: controller
+  namespace: syste  m


deploy/kube-app-manager-aio.yaml

e2e/main_test.go

barney-s · 2020-02-27T07:57:49Z

/assign @barney-s

Liujingfang1 · 2020-02-27T17:26:58Z

config/default/kustomization.yaml

 commonLabels:
  control-plane: kube-app-manager
+  app.kubernetes.io/name: kube-app-manager
+  app.kubernetes.io/version: dev


Please don't add app.kubernetes.io/version: dev as a commonLabel. The commonLabel added here will be injected to the labelSelector of the StatefulSet for the manager. It will cause trouble when you update the manager to a different version.

Suggest to add it in the commonAnnotation.

Good catch, thank you !

app.kubernetes.io/version is one of the 5-6 recommended label as suggested by the kubernetes.io documentation.
I’m removing it, but I think it could also be a potential improvement of kustomize: not all labels are there for label selection, some are used for filtering/indexing/operator management.
I’ll open an issue on kustomize repo to followup on this.

barney-s · 2020-02-28T17:12:01Z

@ant31 - thanks for this change. once it is merged iam thinking we could generate a release artifact that includes the all-in-one deploy yaml.

barney-s · 2020-03-05T23:18:46Z

@ant31 - Would you please address the review comments. Let us know when we can review again.

ant31 · 2020-03-07T00:02:04Z

I went through the previous comments.

created overlay per version (instead of using 'sed')
directly use the aoi file in the make (un)deploy targets)
removed the version from common labels

for now I keep the image on quay, next week I ll look at moving it to k8s. gcr. io

Thank you for the review

ant31 · 2020-03-07T15:34:16Z

Makefile

 .PHONY: install
-install: $(TOOLBIN)/kustomize $(TOOLBIN)/kubectl
+deploy-crd: $(TOOLBIN)/kustomize $(TOOLBIN)/kubectl
 	$(TOOLBIN)/kustomize build config/crd| $(TOOLBIN)/kubectl apply -f -


install vs deploy wording is confusing in kubernetes ecosystem . Especially in this case when it's a substep of the whole deployment.
Renamed it in deploy-crd

ant31 · 2020-03-07T15:36:24Z

Makefile


 .PHONY: docker-build
-docker-build: test $(TOOLBIN)/kustomize ## Build the docker image for kube-app-manager
+docker-build: set-image test $(TOOLBIN)/kustomize ## Build the docker image for kube-app-manager


Image is set only when it's a custom image, otherwise it's already configured in the versions overlay

ant31 · 2020-03-10T09:29:00Z

@barney-s it's ready for a second pass.
Also, unless there's some additional blockers, I suggest to merge and I'll address remaining comment promptly in a followup PR, it will be easier to review small changes than re-review on this large PR.

barney-s · 2020-03-11T22:06:21Z

/approve
/lgtm

Thanks @ant31

k8s-ci-robot · 2020-03-11T22:06:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ant31, barney-s

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [ant31,barney-s]

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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 24, 2020

k8s-ci-robot requested review from mattfarina and prydonius February 24, 2020 14:26

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 24, 2020

This was referenced Feb 24, 2020

Rename kube app manager #176

Closed

Why is there a Webhook configuration ? #175

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 25, 2020

ant31 added 2 commits February 26, 2020 14:13

Create all-in-one file to deploy kube-app-manager

bd3f605

Change the e2e application sample path

9aaf167

ant31 force-pushed the create_aio branch from 3ba4287 to 9aaf167 Compare February 26, 2020 13:13

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2020

ant31 commented Feb 26, 2020

View reviewed changes

deploy/kube-app-manager-aio.yaml Show resolved Hide resolved

barney-s reviewed Feb 27, 2020

View reviewed changes

k8s-ci-robot assigned barney-s Feb 27, 2020

Liujingfang1 reviewed Feb 27, 2020

View reviewed changes

ant31 mentioned this pull request Mar 5, 2020

Fix directory name following a rename #182

Closed

ant31 commented Mar 7, 2020

View reviewed changes

One kustomize-overlay per version

76d7c2e

ant31 force-pushed the create_aio branch from de5adeb to 76d7c2e Compare March 9, 2020 12:02

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2020

k8s-ci-robot merged commit c9672ae into kubernetes-sigs:master Mar 11, 2020

Create all-in-one file to deploy kube-app-manager #179

Create all-in-one file to deploy kube-app-manager #179

Conversation

ant31 commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

barney-s commented Feb 25, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

barney-s commented Feb 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barney-s commented Feb 28, 2020

Uh oh!

barney-s commented Mar 5, 2020

Uh oh!

ant31 commented Mar 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ant31 commented Mar 10, 2020

Uh oh!

barney-s commented Mar 11, 2020

Uh oh!

k8s-ci-robot commented Mar 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ant31 commented Feb 24, 2020 •

edited

Loading