Adding JFrog Artifactory #142

jainishshah17 · 2016-10-21T21:02:32Z

No description provided.

k8s-ci-robot · 2016-10-21T21:02:33Z

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot ok to test" on its own line.

Regular contributors should join the org to skip this step.

googlebot · 2016-10-21T21:02:35Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

viglesiasce · 2016-10-21T21:09:13Z

Hey @jainishshah17!!

Thanks for the submission!

A few things to fix up:

The nginx chart was errantly checked in
You need to sign the Linux Foundation CLA as noted above
The .gitignore change has already been checked in
Use a ClusterIP service by default, we prefer not to optimize for Minikube
Does this chart work out of the box without having a pro license?
You have a note about volumes and config maps in the deployment but I dont see that configuration included.

googlebot · 2016-10-22T22:27:51Z

CLAs look good, thanks!

ultimateboy · 2016-10-24T21:07:56Z

incubator/artifactory/README.md

+
+## Configuration
+
+The following tables lists the configurable parameters of the elasticsearch chart and their default values.


s/elasticsearch/JFrog Artifactory

chrislovecnm · 2016-10-26T02:09:13Z

Can u squash your commits amigo?

chrislovecnm · 2016-10-26T02:12:02Z

@jainishshah17 your rebase is a tad wonky. Please remove the .ignore changes out of the commit ;)

chrislovecnm

Thanks for the PR!! Have a couple tweaks

chrislovecnm · 2016-10-26T02:13:39Z

incubator/artifactory/README.md

+## Prerequisites Details
+
+* Kubernetes 1.3 with alpha APIs enabled
+* Artifactory Pro trial license


@viglesiasce how are we handling paid for software? Do we need some notice in the README, hey you need to pay for this ;)

chrislovecnm · 2016-10-26T02:15:40Z

incubator/artifactory/README.md

+
+```
+$ kubectl delete pods -l release=my-release,type=data
+$ kubectl delete pvcs -l release=my-release,type=data


Can you include the timed wait that is in the PetSet docs?

@chrislovecnm I have updated my chart

No here is an example

$ grace=$(kubectl get po web-0 --template '{{.spec.terminationGracePeriodSeconds}}') $ kubectl delete petset,po -l app=nginx $ sleep $grace $ kubectl delete pvc -l app=nginx

chrislovecnm · 2016-10-26T02:17:49Z

incubator/artifactory/templates/deployment.yaml

+        ports:
+        - containerPort: 8081
+          name: http
+        # This (and the volumes section below) mount the config map as a volume.


Where is this at? We need resource limit and health probes as well please

Changed Servie type to NodePort for minikube Removed Dependencies Updated Readme Removed Nginx Updated Readme Added Volume and Resource removed gitignore file

prydonius · 2016-11-04T18:39:49Z

@jainishshah17 any luck on the Linux Foundation CLA?

jainishshah17 · 2016-11-04T19:00:46Z

@prydonius Done signing it.

prydonius · 2016-11-04T20:21:58Z

Can you rebase to fix the conflicts?

chrislovecnm

Thanks for the chart. Some more notes.

chrislovecnm · 2016-11-06T00:13:03Z

.gitignore

@@ -1,35 +0,0 @@
-# General files for the project


You need to remove this file from your commit. You are removing it from the repo. Add it back in your branch please.

chrislovecnm · 2016-11-06T00:14:59Z

incubator/artifactory/README.md

+
+```
+$ kubectl delete pods -l release=my-release,type=data
+$ kubectl delete pvcs -l release=my-release,type=data


No here is an example

$ grace=$(kubectl get po web-0 --template '{{.spec.terminationGracePeriodSeconds}}') $ kubectl delete petset,po -l app=nginx $ sleep $grace $ kubectl delete pvc -l app=nginx

chrislovecnm · 2016-11-06T00:15:53Z

incubator/artifactory/templates/deployment.yaml

+        # is a nice option for the user. Especially in the strange cases like
+        # artifactory where the base distro is determined by the tag. Using :latest
+        # is frowned upon, using :stable isn't that great either.
+        image: "{{default "docker.bintray.io/jfrog/artifactory-pro" .Values.image}}:{{default "4.13.2" .Values.imageTag}}"


I would prefer to have these hard coded values in values.yaml. @viglesiasce thoughts?

I agree. Also, fix to .Values.Image (capital i) and .Values.ImageTag, as in values.yaml.

chrislovecnm · 2016-11-06T00:17:41Z

incubator/artifactory/templates/deployment.yaml

+        # is frowned upon, using :stable isn't that great either.
+        image: "{{default "docker.bintray.io/jfrog/artifactory-pro" .Values.image}}:{{default "4.13.2" .Values.imageTag}}"
+        imagePullPolicy: {{default "IfNotPresent" .Values.pullPolicy}}
+        resources:


This is a much cleaner way to do the resources https://github.com/kubernetes/charts/blob/master/stable/mysql/templates/deployment.yaml#L34

You will need the https://github.com/kubernetes/charts/blob/master/stable/mysql/templates/_helpers.tpl file as well in your chart.

chrislovecnm · 2016-11-06T00:19:34Z

incubator/artifactory/templates/deployment.yaml

+          name: {{ template "fullname" . }}
+        - name: logs
+        configMap:
+          name: {{ template "fullname" . }}


You list PVC in your instructions, but you do not define any PVC components in your chat?

alon-argus · 2016-11-10T07:32:01Z

incubator/artifactory/values.yaml

@@ -0,0 +1,17 @@
+# Default values for etcd.


alon-argus · 2016-11-10T07:45:32Z

incubator/artifactory/templates/deployment.yaml

+        # artifactory where the base distro is determined by the tag. Using :latest
+        # is frowned upon, using :stable isn't that great either.
+        image: "{{default "docker.bintray.io/jfrog/artifactory-pro" .Values.image}}:{{default "4.13.2" .Values.imageTag}}"
+        imagePullPolicy: {{default "IfNotPresent" .Values.pullPolicy}}


.Values.pullPolicy -> .Values.ImagePullPolicy

alon-argus · 2016-11-10T07:48:41Z

incubator/artifactory/templates/svc.yaml

+    chart: "{{.Chart.Name}}-{{.Chart.Version}}"
+spec:
+  ports:
+  - port: {{default 8081 .Values.httpPort}}


.Values.httpPort is not defined

viglesiasce · 2016-11-14T16:46:56Z

Can you add a NOTES.txt file that explains what users need to do in order to use the Chart?

jainishshah17 · 2016-11-30T17:29:04Z

@prydonius I have updated my changes. Please check

viglesiasce · 2016-12-02T06:51:22Z

@k8s-bot ok to test

viglesiasce · 2016-12-02T06:54:03Z

stable/mysql/values.yaml

@@ -31,7 +31,7 @@ imageTag: "5.7.14"
 persistence:
  enabled: true
  storageClass: generic
-  accessMode: ReadWriteOnce
+  accessMode: ReadWrite


This should not be changed in this PR.

Changed default image to Artifactory-oss

viglesiasce · 2016-12-02T06:54:16Z

incubator/artifactory/values.yaml

+persistence:
+  enabled: true
+  storageClass: generic
+  accessMode: ReadWrite


Invalid access mode, see test results.

viglesiasce · 2016-12-02T06:54:33Z

incubator/artifactory/values.yaml

+
+Name: artifactory
+Component: "Artifactory"
+image: "docker.bintray.io/jfrog/artifactory-pro"


can we default this to the oss image and have a note about how to switch it?

jainishshah17 · 2016-12-04T13:23:53Z

@k8s-bot e2e test this

k8s-ci-robot · 2016-12-04T13:23:54Z

@jainishshah17: you can't request testing unless you are a kubernetes member.

In response to this comment:

@k8s-bot e2e test this

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

viglesiasce · 2016-12-05T18:33:35Z

@k8s-bot ok to test

viglesiasce · 2016-12-05T18:37:52Z

incubator/artifactory/templates/NOTES.txt

@@ -0,0 +1,6 @@
+Artifactory can be accessed via port 8081 on the following DNS name from within your cluster:
+{{ template "fullname" . }}.{{ .Release.Namespace }}.svc.cluster.local
+


Add examples of how to connect to this from outside the cluster. Check out the jenkins chart for an example of how to differentiate between the various service modes.

viglesiasce · 2016-12-05T18:39:07Z

incubator/artifactory/values.yaml

+## Kubernetes configuration
+## For minikube, set this to NodePort, elsewhere use LoadBalancer
+##
+ServiceType: LoadBalancer


Lets make this ClusterIP by default since the password isnt randomly generated by default.

viglesiasce · 2016-12-05T18:39:56Z

Looking good @jainishshah17! Was able to install it this time around. 2 more things to fix:

Add NOTES about how to connect from outside the cluster
Make the service ClusterIP by default

k8s-ci-robot · 2016-12-10T16:38:46Z

Jenkins Charts e2e failed for commit 19780eb. Full PR test history.

The magic incantation to run this job again is @k8s-bot e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

jainishshah17 · 2016-12-11T07:38:48Z

@viglesiasce Made changes please check

viglesiasce · 2016-12-19T23:18:29Z

This worked for me. Thanks @jainishshah17!

[T2A-143] use PAPERTRAIL_SYSTEM_NAME for hostname

Adding JFrog Artifactory

googlebot added the cla: no label Oct 21, 2016

googlebot added cla: yes and removed cla: no labels Oct 22, 2016

ultimateboy reviewed Oct 24, 2016

View reviewed changes

chrislovecnm suggested changes Oct 26, 2016

View reviewed changes

Adding Artifactory-Pro Chart

e8ebe83

Changed Servie type to NodePort for minikube Removed Dependencies Updated Readme Removed Nginx Updated Readme Added Volume and Resource removed gitignore file

jainishshah17 force-pushed the master branch from 18f558c to e8ebe83 Compare October 28, 2016 22:57

Link to get trial license

a89911a

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

f340230

chrislovecnm suggested changes Nov 6, 2016

View reviewed changes

reading resources from Values

2f6039d

alon-argus reviewed Nov 10, 2016

View reviewed changes

incubator/artifactory/values.yaml

@@ -0,0 +1,17 @@

# Default values for etcd.

Copy link

alon-argus Nov 10, 2016

Choose a reason for hiding this comment

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

etcd?

alon-argus reviewed Nov 10, 2016

View reviewed changes

Fixed Typos

9bbaeac

prydonius added the changes needed label Nov 21, 2016

Merge remote-tracking branch 'k8s/master'

b6eea86

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 23, 2016

jainishshah17 added 2 commits November 23, 2016 14:34

try

0e3ba3b

Removing Index.yaml file

392207b

viglesiasce added awaiting review and removed changes needed labels Dec 2, 2016

viglesiasce suggested changes Dec 2, 2016

View reviewed changes

jainishshah17 added 2 commits December 4, 2016 09:22

Reverted back changes in mysql

c80ec97

Changed default image to Artifactory-oss image

98e31cb

fixed Access

bc17bf6

viglesiasce suggested changes Dec 5, 2016

View reviewed changes

viglesiasce added changes needed and removed awaiting review labels Dec 5, 2016

Made changes according to recommendation

19780eb

Reverted value of jenkins serviceType

cab7741

viglesiasce added awaiting review and removed changes needed labels Dec 19, 2016

viglesiasce approved these changes Dec 19, 2016

View reviewed changes

viglesiasce added code reviewed lgtm Indicates that a PR is ready to be merged. UX reviewed and removed awaiting review labels Dec 19, 2016

prydonius merged commit c265c9a into helm:master Dec 19, 2016

jordanjennings pushed a commit to jordanjennings/charts that referenced this pull request Nov 27, 2018

Merge pull request helm#142 from nkiraly/T2A-143/fix-papertrail-hostname

ff5a006

[T2A-143] use PAPERTRAIL_SYSTEM_NAME for hostname

wmcdona89 pushed a commit to wmcdona89/charts that referenced this pull request Aug 30, 2020

Merge pull request helm#142 from jainishshah17/master

a0365dc

Adding JFrog Artifactory


		## Configuration

		The following tables lists the configurable parameters of the elasticsearch chart and their default values.

		@@ -0,0 +1,6 @@
		Artifactory can be accessed via port 8081 on the following DNS name from within your cluster:
		{{ template "fullname" . }}.{{ .Release.Namespace }}.svc.cluster.local

Adding JFrog Artifactory #142

Adding JFrog Artifactory #142

Conversation

jainishshah17 commented Oct 21, 2016

k8s-ci-robot commented Oct 21, 2016

googlebot commented Oct 21, 2016

viglesiasce commented Oct 21, 2016

googlebot commented Oct 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrislovecnm commented Oct 26, 2016

chrislovecnm commented Oct 26, 2016

chrislovecnm 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

prydonius commented Nov 4, 2016

jainishshah17 commented Nov 4, 2016

prydonius commented Nov 4, 2016

chrislovecnm 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

alon-argus Nov 10, 2016 • edited Loading

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

viglesiasce commented Nov 14, 2016

jainishshah17 commented Nov 30, 2016

viglesiasce commented Dec 2, 2016

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 Dec 4, 2016

k8s-ci-robot commented Dec 4, 2016

viglesiasce commented Dec 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viglesiasce commented Dec 5, 2016

k8s-ci-robot commented Dec 10, 2016

jainishshah17 commented Dec 11, 2016

viglesiasce commented Dec 19, 2016

alon-argus Nov 10, 2016 •

edited

Loading