Moved Artifactory to stable and updated version to 5.3.2 #1314

jainishshah17 · 2017-06-16T21:40:59Z

No description provided.

# Conflicts: # incubator/artifactory/Chart.yaml

# Conflicts: # incubator/artifactory/Chart.yaml # incubator/artifactory/README.md # incubator/artifactory/values.yaml

k8s-ci-robot · 2017-06-16T21:41:07Z

Hi @jainishshah17. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

unguiculus

Thanks for the update. I made a few comments.

unguiculus · 2017-06-18T16:11:02Z

stable/artifactory/README.md

+
+|         Parameter         |           Description             |                         Default                          |
+|---------------------------|-----------------------------------|----------------------------------------------------------|
+| `art_image.version`       | Container image tag               | `5.3.2`                                                 |


Documentation incomplete.

unguiculus · 2017-06-18T16:13:09Z

stable/artifactory/templates/artifactory-deployment.yaml

+  labels:
+    chart: "{{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}"
+spec:
+  replicas: {{ .Values.art_replicaCount }}


Don't use underscores. Use nesting instead.

See https://github.com/kubernetes/helm/blob/master/docs/chart_best_practices/values.md

Again, a good example would be https://github.com/kubernetes/charts/tree/master/stable/nginx-ingress/.

unguiculus · 2017-06-18T16:15:37Z

stable/artifactory/templates/artifactory-deployment.yaml

+metadata:
+  name: {{ .Values.art_name }}
+  labels:
+    chart: "{{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}"


Add standard labels. Apply everywhere. Here's a good example:

https://github.com/kubernetes/charts/tree/master/stable/nginx-ingress/

name should be {{ template "fullname" . }}

app should be {{ template "name" . }}

Adapt as in nginx-ingress example using components. You will also have to update selectors in services.

unguiculus · 2017-06-18T16:25:20Z

stable/artifactory/templates/artifactory-deployment.yaml

+        pod.beta.kubernetes.io/init-containers: '[{
+          "name": "remove-lost-found",
+          "image": {{ .Values.initContainerImage | quote }},
+          "command": ["rm", "-rf", "{{ .Values.art_persistence.mountPath }}/lost+found"],


Why would you want to do this? Sounds like a hack to me. If lost+found gets in your way, why don't you use a subdirectory?

We do have 6 subdirectories and few files.

jainishshah17 · 2017-06-20T15:20:48Z

@unguiculus Made suggested changes please check.

unguiculus · 2017-06-20T15:28:05Z

The app label is not yet correct everywhere.

May I direct you to the nginx-ingress example again regarding resource names:
https://github.com/kubernetes/charts/blob/master/stable/nginx-ingress/templates/controller-deployment.yaml#L11
https://github.com/kubernetes/charts/blob/master/stable/nginx-ingress/templates/_helpers.tpl#L18-L25

jainishshah17 · 2017-06-23T06:36:23Z

@unguiculus Does it looks good now?

unguiculus · 2017-06-23T09:19:52Z

stable/artifactory/Chart.yaml

@@ -1,8 +1,8 @@
 apiVersion: v1
 name: artifactory
 home: https://www.jfrog.com/artifactory/
-version: 5.3.2
-appVersion: 5.3.2
+version: 5.4.1


Chart version and app version are two different things. They don't have to be aligned.

unguiculus · 2017-06-23T09:25:58Z

Now it is kind of messed up. Don't confuse name and the app label. Let me again suggest you to study the nginx-ingress chart as mentioned above. It has multiple components as well. Check how they are structured, named, and labeled.

https://github.com/kubernetes/charts/tree/master/stable/nginx-ingress
https://github.com/kubernetes/charts/blob/master/stable/nginx-ingress/templates/_helpers.tpl

unguiculus · 2017-06-28T10:03:34Z

stable/artifactory/templates/artifactory-pvc.yaml

+metadata:
+  name: {{ template "artifactory.fullname" . }}
+  labels:
+    app: {{ template "fullname" . }}


app: {{ template "name" . }}

unguiculus · 2017-06-28T10:04:10Z

stable/artifactory/templates/nginx-pvc.yaml

+metadata:
+  name: {{ template "nginx.fullname" . }}
+  labels:
+    app: {{ template "fullname" . }}


app: {{ template "name" . }}

unguiculus · 2017-06-28T10:04:49Z

stable/artifactory/templates/nginx-service.yaml

+    targetPort: {{ .Values.nxService.internalPortHttps }}
+    protocol: TCP
+    name: {{ .Values.nxService.name }}https
+  selector:


Add release: {{ .Release.Name }} to selector.

unguiculus · 2017-06-28T10:05:22Z

stable/artifactory/templates/postgresql-pvc.yaml

+metadata:
+  name: {{ template "database.fullname" . }}
+  labels:
+    app: {{ template "fullname" . }}


app: {{ template "name" . }}

unguiculus · 2017-06-28T10:05:44Z

stable/artifactory/templates/postgresql-service.yaml

+    targetPort: {{ .Values.dbService.internalPort }}
+    protocol: TCP
+    name: {{ .Values.dbService.name }}
+  selector:


Add release: {{ .Release.Name }} to selector.

unguiculus · 2017-06-28T10:06:37Z

stable/artifactory/templates/artifactory-service.yaml

+    targetPort: {{ .Values.artService.internalPort }}
+    protocol: TCP
+    name: {{ .Values.artService.name }}
+  selector:


Add release: {{ .Release.Name }} to selector.

unguiculus · 2017-06-28T10:11:26Z

stable/artifactory/templates/NOTES.txt

+   echo http://$SERVICE_IP/
+
+   {{- else if contains "ClusterIP" .Values.artService.type }}
+   export POD_NAME=$(kubectl get pods --namespace {{ .Release.Namespace }} -l "app={{ .Values.artService.name }}" -o jsonpath="{.items[0].metadata.name}")


Add release and component labels to the selector.

Also, use the templates which use the values instead of the values directly. E. g. use {{ template "artifactory.name" . }} instead of {{ .Values.artService.name }}. Apply throughout NOTES.txt.

jainishshah17 · 2017-06-30T04:00:12Z

@unguiculus does it looks good now?

unguiculus

Almost there. Thanks you.

unguiculus · 2017-06-30T06:37:37Z

stable/artifactory/templates/artifactory-service.yaml

+    targetPort: {{ .Values.artService.internalPort }}
+    protocol: TCP
+    name: {{ .Release.Name }}
+  selector:


Add release: {{ .Release.Name }} to selector.

unguiculus · 2017-06-30T06:37:49Z

stable/artifactory/templates/nginx-service.yaml

+    targetPort: {{ .Values.nxService.internalPortHttps }}
+    protocol: TCP
+    name: {{ .Release.Name }}https
+  selector:


Add release: {{ .Release.Name }} to selector.

unguiculus · 2017-06-30T06:38:51Z

stable/artifactory/templates/postgresql-service.yaml

+    targetPort: {{ .Values.dbService.internalPort }}
+    protocol: TCP
+    name: {{ .Release.Name }}
+  selector:


Add release: {{ .Release.Name }} to selector.

jainishshah17 · 2017-06-30T06:47:30Z

@unguiculus done

unguiculus · 2017-07-01T15:44:55Z

stable/artifactory/Chart.yaml

@@ -11,4 +13,6 @@ sources:
 maintainers:
  - name: Jainish shah
    email: jainishs@jfrog.com
+  - name: Eldad Assis


Please use the Github username as name. We've started to recommend that because it helps identify the chart maintainer for future review.

unguiculus · 2017-07-01T15:45:38Z

@k8s-bot ok to test

unguiculus · 2017-07-01T15:55:02Z

stable/artifactory/README.md

+**NOTE:** The Artifactory OSS does not use Nginx, so your Artifactory's Kubernetes service exposes Tomcat's port 8081 as port 80.
+
+By default it will run Artifactory-pro to run Artifactory-oss comment nginx configuration in value.yaml and use following command:
+Remove `nginx-deployment.yaml`, `nginx-pvc.yaml` and `nginx-service.yaml` and change art_service.externalPort to 80 in [values.yaml](values.yaml) before running command to install Artifactory-oss.


This is not ideal. You cannot just remove something when you install the chart from the repo. The user would have to download and adapt it. Please add some conditional logic to not include what's not needed for OSS. BTW, why wouldn't you run the OSS version behind Nginx?

unguiculus

I'm sorry to be a little picky, but I'd really prefer it if you properly nested config values like so:

database:
  replicaCount: 1
  type: postgresql
  name: artifactory
  user: artifactory
  pass: artXifactory1973
  image:
    repository: docker.bintray.io/postgres
    tag: 9.5.2
    pullPolicy: IfNotPresent
  service:
    type: ClusterIP
    ...
  persistence:
    ...

artifactory:
  replicaCount:
  image:
    repository: docker.bintray.io/jfrog/artifactory-pro
    tag: 5.4.2
    pullPolicy: IfNotPresent
  service:
    ...
  persistence:
    ...

nginx:
  image:
    ...
  service:
    ...

Would you mind changing that, please?

Also, you should use secrets for credentials.

Other than that, I managed to successfully test the chart on GKE.

unguiculus · 2017-07-01T15:59:11Z

stable/artifactory/templates/artifactory-service.yaml

+  labels:
+    app: {{ template "name" . }}
+    chart: {{ .Chart.Name }}-{{ .Chart.Version }}
+    component: "{{ .Values.art_name }}"


art_name -> artName

unguiculus · 2017-07-01T15:59:30Z

stable/artifactory/templates/artifactory-deployment.yaml

+  labels:
+    app: {{ template "name" . }}
+    chart: {{ .Chart.Name }}-{{ .Chart.Version }}
+    component: "{{ .Values.art_name }}"


art_name -> artName

unguiculus · 2017-07-01T16:00:47Z

stable/artifactory/README.md

+**NOTE:** The Artifactory OSS does not use Nginx, so your Artifactory's Kubernetes service exposes Tomcat's port 8081 as port 80.
+
+By default it will run Artifactory-pro to run Artifactory-oss comment nginx configuration in value.yaml and use following command:
+Remove `nginx-deployment.yaml`, `nginx-pvc.yaml` and `nginx-service.yaml` and change art_service.externalPort to 80 in [values.yaml](values.yaml) before running command to install Artifactory-oss.


art_service-> artService

unguiculus · 2017-07-01T16:01:22Z

stable/artifactory/README.md

+| `dbPersistence.accessMode` | Database persistence volume access mode | `ReadWriteOnce`   |
+| `dbPersistence.size` | Database persistence volume size | `10Gi`   |
+| `artName` | Artifactory name | `artifactory`   |
+| `artName.art_replicaCount` | Artifactory replica count | `1`   |


artName.art_replicaCount -> artName.artReplicaCount

unguiculus · 2017-07-01T16:04:15Z

stable/artifactory/values.yaml

+# Access the values with {{ .Values.key.subkey }}
+
+# Common
+replicaCount: 1


This is not common. It's used for the DB. You should prefix this as well.

unguiculus · 2017-07-01T16:06:12Z

stable/artifactory/README.md

+
+|         Parameter         |           Description             |                         Default                          |
+|---------------------------|-----------------------------------|----------------------------------------------------------|
+| `replicaCount`            | Replica count for Artifactory deployment| `1`                                                |


Wrong. Used for DB. See below.

unguiculus · 2017-07-05T15:48:58Z

Have you seen my comment regarding secrets?

jainishshah17 · 2017-07-05T17:01:35Z

@unguiculus done!

unguiculus · 2017-07-05T20:02:37Z

stable/artifactory/templates/secret.yaml

+kind: Secret
+metadata:
+  labels:
+    name: {{ template "name" . }}


name label should be app label.

unguiculus · 2017-07-05T20:05:29Z

stable/artifactory/templates/secret.yaml

@@ -0,0 +1,16 @@
+apiVersion: v1


I'd suggest you rename it to postgresql-secret.yaml.

jainishshah17 · 2017-07-05T20:08:35Z

@unguiculus changed

* upstream/master: (67 commits) Fix json whitespace (helm#1458) Use consistent whitespace in template placeholders (helm#1437) [stable/selenium] Make hub readiness probe timeout configurable (helm#1391) [stable/kube2iam]: add rbac support (helm#1286) [stable/traefik] Allow enabling traefik access logs (helm#1302) Add Stash chart (helm#1420) Add Gearman G2 chart (helm#1421) add option to include tolerations to daemonset (helm#1364) Moved Artifactory to stable and updated version to 5.3.2 (helm#1314) Concourse postgres conditional dependency (helm#1390) Typo in helm install command for dask-distributed (helm#1413) [stable/fluent-bit] Fluent Bit v0.11.12 (helm#1417) fixed cassandra chart's persistence bug (helm#1245) Prometheus: modify config to support k8s 1.6 by default (helm#1080) Add rocket.chat (helm#752) Fix influxdb deployment (helm#1424) feat(stable/etcd-operator): add support for supplying additional command args (helm#1418) add configurable service annotations helm#1234 (helm#1244) [stable/prometheus] extra environment variable for alert manager (helm#1237) [stable/heapster] Default service name to Heapster (helm#1266) ...

* Upgraded artifactory version to 4.16.0 * changes as per suggestion * Added Stable Artifactory chart * removed Incubator Artifactory chart * Fixed port issue * Fixed path * fixed readme * Added Documentation for all variables. * Updated labels * Fixed Parameter names * Fixed Notes.txt * Updated Artifactory version to 5.4.1 * Fixed metadata * Fixed naming * Changes. * Added release name to selector * Nginx for Oss and Pro * Fixed readme * using secret for database password * renamed secret to postgresql-secret.yaml

* Automated cherry pick of helm#1314 (cert-manager/cert-manager#1315) * Automated cherry pick of helm#1294 (cert-manager/cert-manager#1296) * Automated cherry pick of helm#1276 (cert-manager/cert-manager#1277) * Automated cherry pick of helm#1258 helm#1266 (cert-manager/cert-manager#1273) * Automated cherry pick of helm#1259 (cert-manager/cert-manager#1260) * Update Chart.yaml in webhook (cert-manager/cert-manager#1249)

* Automated cherry pick of helm#1314 (cert-manager/cert-manager#1315) * Automated cherry pick of helm#1294 (cert-manager/cert-manager#1296) * Automated cherry pick of helm#1276 (cert-manager/cert-manager#1277) * Automated cherry pick of helm#1258 helm#1266 (cert-manager/cert-manager#1273) * Automated cherry pick of helm#1259 (cert-manager/cert-manager#1260) * Update Chart.yaml in webhook (cert-manager/cert-manager#1249) Signed-off-by: James Munnelly <james@munnelly.eu>

jainishshah17 added 7 commits January 19, 2017 16:35

Upgraded artifactory version to 4.16.0

266a42b

Merge branch 'master' of https://github.com/kubernetes/charts

dc1f0cd

# Conflicts: # incubator/artifactory/Chart.yaml

changes as per suggestion

f22f5cd

Merge remote-tracking branch 'k8s/master'

73d4a02

Merge remote-tracking branch 'Charts/master'

54834d8

# Conflicts: # incubator/artifactory/Chart.yaml # incubator/artifactory/README.md # incubator/artifactory/values.yaml

Added Stable Artifactory chart

5b9e47b

removed Incubator Artifactory chart

c7fd27c

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 16, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 16, 2017

jainishshah17 added 3 commits June 16, 2017 20:31

Fixed port issue

7312c59

Fixed path

acdffb2

fixed readme

ddbd6a8

unguiculus suggested changes Jun 18, 2017

View reviewed changes

unguiculus self-assigned this Jun 18, 2017

unguiculus requested a review from mgoodness June 18, 2017 16:30

jainishshah17 added 4 commits June 19, 2017 10:25

Added Documentation for all variables.

34ce7ff

Updated labels

c02d548

Fixed Parameter names

9e2c8a1

Fixed Notes.txt

fb226db

jainishshah17 added 2 commits June 22, 2017 17:24

Updated Artifactory version to 5.4.1

7e68a82

Fixed metadata

3e89133

unguiculus reviewed Jun 23, 2017

View reviewed changes

jainishshah17 added 2 commits June 25, 2017 11:45

Merge remote-tracking branch 'Charts/master'

dc99e38

Fixed naming

d6aeb9d

unguiculus suggested changes Jun 28, 2017

View reviewed changes

jainishshah17 added 2 commits June 29, 2017 11:00

Changes.

577b918

Merge remote-tracking branch 'Charts/master'

bac7bcb

unguiculus suggested changes Jun 30, 2017

View reviewed changes

Added release name to selector

a0571f1

unguiculus reviewed Jul 1, 2017

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 1, 2017

unguiculus reviewed Jul 1, 2017

View reviewed changes

unguiculus added size/large UX reviewed labels Jul 1, 2017

unguiculus suggested changes Jul 1, 2017

View reviewed changes

jainishshah17 added 2 commits July 3, 2017 15:13

Nginx for Oss and Pro

7f3f936

Fixed readme

f5e02b2

using secret for database password

4a788bd

unguiculus mentioned this pull request Jul 5, 2017

Use consistent whitespace in template placeholders #1437

Merged

unguiculus suggested changes Jul 5, 2017

View reviewed changes

renamed secret to postgresql-secret.yaml

e55e040

unguiculus added code reviewed lgtm Indicates that a PR is ready to be merged. labels Jul 6, 2017

unguiculus merged commit 689cc8b into helm:master Jul 6, 2017

Moved Artifactory to stable and updated version to 5.3.2 #1314

Moved Artifactory to stable and updated version to 5.3.2 #1314

Conversation

jainishshah17 commented Jun 16, 2017

k8s-ci-robot commented Jun 16, 2017

unguiculus 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

jainishshah17 commented Jun 20, 2017

unguiculus commented Jun 20, 2017

jainishshah17 commented Jun 23, 2017

Choose a reason for hiding this comment

unguiculus commented Jun 23, 2017

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

jainishshah17 commented Jun 30, 2017

unguiculus 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

jainishshah17 commented Jun 30, 2017

Choose a reason for hiding this comment

unguiculus commented Jul 1, 2017

Choose a reason for hiding this comment

unguiculus 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

unguiculus commented Jul 5, 2017

jainishshah17 commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jainishshah17 commented Jul 5, 2017