SQL Server 2017 Linux HELM Chart by thomasliddledba · Pull Request #3246 · helm/charts

thomasliddledba · 2018-01-05T22:10:14Z

Adding a SQL Server 2017 Linux HELM Chart to the incubator.

thomasliddledba · 2018-01-05T22:21:22Z

thomasliddledba · 2018-01-05T22:22:13Z

/approve

thomasliddledba · 2018-01-06T12:58:23Z

Note - I read through issues #1085 and #1733. I'm hoping for another review. With GA now for SQL Server 2017 on Linux, this charts Prerequisites to change the EULA value, and the charts default to Express, there would be another review and acceptance. I believe there is value in a chart for SQL Server 2017 for the community.

…sqldevelop

thomasliddledba · 2018-01-10T02:06:36Z

Any update on this PR?

foxish

Some initial feedback.
/cc @lachie83

foxish · 2018-01-11T13:29:06Z

incubator/mssql-linux/README.md

+$ echo -n "<MyNewStrongSaPassword>"| base64
+$ <base 64 output>
+```
+3.  Copy the Base64 output in step 2 into the templates/secret.yaml in the `password` field


We can do this in the templating similar to https://github.com/kubernetes/charts/blob/master/stable/mysql/templates/secrets.yaml

This was a great idea. I added this to the commit.

foxish · 2018-01-11T13:31:35Z

incubator/mssql-linux/README.md

+# HELM Chart for Microsoft SQL Server 2017 on Linux
+
+## Prerequisites
+ * This chart requires Docker Engine 1.8+ in any of their supported platforms.


This is a bit atypical - why does it have a docker engine version dependency?

There was a prerequisite in the SQL Server documentation for 1.8 or higher. I updated the README file with a link to the documentation.

foxish · 2018-01-11T13:32:42Z

incubator/mssql-linux/README.md

+```
+
+## Values
+The configuration parameters in this section control the resources requested and utilized by the ZooKeeper ensemble.


foxish · 2018-01-11T13:35:02Z

incubator/mssql-linux/README.md

+| type | Service Type | ClusterIP |
+| externalPort | External Port | 1433 |
+| internalPort | Internal Port | 1433 |
+| livenessprobe | | |


Either liveness and readiness probes should be separate tables, or the parameters should be described as livenessprobe.initialDelaySeconds and so on.

Fixed. I put this in it's own table(s)

foxish · 2018-01-11T13:35:29Z

incubator/mssql-linux/README.md

+| periodSeconds | Field specifies that the kubelet should perform a liveness probe every XX second(s) | 10 | 
+
+## Resources
+Typically don't recommend specifying default resources and to leave this as a conscious


Please rephrase this - if it's intended towards the user, it can be "You don't typically need to specify..."

foxish · 2018-01-11T13:36:32Z

incubator/mssql-linux/README.md

+lines, adjust them as necessary, and remove the curly braces after 'resources:'.
+
+The defaults are set for local development purposes.
+```yaml


Why not template this also?

Can you elaborate on what to put as a template? It's not clear on the comment. Thanks.

foxish · 2018-01-11T13:38:18Z

/ok-to-test

thomasliddledba · 2018-01-15T11:42:22Z

@lachie83 - I'm sure your busy but would you be able to review this PR for approval?

unguiculus · 2018-01-15T11:50:57Z

incubator/mssql-linux/templates/deployment.yaml

@@ -0,0 +1,49 @@
+apiVersion: extensions/v1beta1


apps/v1beta2

Thanks for the review. Fixed

unguiculus · 2018-01-15T11:51:57Z

incubator/mssql-linux/templates/secret.yaml

+    heritage: {{ .Release.Service }}
+type: Opaque
+data:
+  password: {{ randAlphaNum 15 | b64enc | quote }}


Please provide an option to explicitly set the password and only auto-generate it if not set.

Thanks for the review. Added

unguiculus · 2018-01-15T11:53:38Z

incubator/mssql-linux/templates/deployment.yaml

+    chart: {{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}
+    release: {{ .Release.Name }}
+    heritage: {{ .Release.Service }}
+spec:


Add spec.selector.

Thanks for the review. Fixed

…yment.yaml

thomasliddledba · 2018-01-17T13:01:02Z

@unguiculus - Thanks for the review. The updates have been made.
@lachie83 - If your not too busy I would appreciate a review.

Thanks everyone!

thomasliddledba · 2018-01-24T11:17:00Z

@unguiculus & @lachie83 any chance for a review?

thomasliddledba · 2018-01-24T11:19:24Z

@foxish is there anything else I can provide you based on your last review?

unguiculus

A few more things. If you like you can directly move it to stable.

unguiculus · 2018-01-27T13:30:28Z

incubator/mssql-linux/README.md

+You can specify the resource limits for this chart in the values.yaml file.
+Example:
+```yaml
+resources:


Fix indentation

Thanks. Fixed

unguiculus · 2018-01-27T13:32:13Z

incubator/mssql-linux/templates/deployment.yaml

+                 name: {{ template "mssql.fullname" . }}-secret
+                 key: sapassword
+          ports:
+            - containerPort: {{ .Values.service.internalPort }}


You should probably hard-code the port. Changing the port will break the chart. Give it a name and use that to reference it from service and probes.

Thanks. Fixed

unguiculus · 2018-01-27T13:34:27Z

incubator/mssql-linux/values.yaml

@@ -0,0 +1,29 @@
+ACCEPT_EULA:


Values should be camel case and start with a lower case letter.

Thanks. Fixed

unguiculus · 2018-01-27T13:35:10Z

incubator/mssql-linux/values.yaml

+# sapassword: "MyStrongPassword1234"
+image:
+  repository: microsoft/mssql-server-linux
+  tag: latest


Use a release version.

Thanks. Fixed

thomasliddledba · 2018-01-29T20:26:45Z

@unguiculus - Thanks for the review. I've corrected those fixes. Any other suggestions would be great. Thanks again.

thomasliddledba · 2018-01-29T20:28:20Z

/assign @lachie83

unguiculus

Looks pretty good. Just a few more things. I'd suggest you move it to stable before we merge it.

unguiculus · 2018-01-29T21:10:43Z

incubator/mssql-linux/templates/service.yaml

+    - port: 1433
+      targetPort: 1433
+      protocol: TCP
+      name: {{ .Values.service.name }}


This is not the service name, it's the port name. It probably doesn't make sense to make it configurable. I'd just hardcode it.

I went ahead and removed this all together.

unguiculus · 2018-01-29T21:11:54Z

incubator/mssql-linux/templates/deployment.yaml

+                 name: {{ template "mssql.fullname" . }}-secret
+                 key: sapassword
+          ports:
+            - containerPort: 1433


Give the port a name and reference it for configuring probes.

Thanks. I've updated the port with a name and referenced it in the probes.

unguiculus · 2018-01-29T21:12:32Z

incubator/mssql-linux/templates/service.yaml

+  type: {{ .Values.service.type }}
+  ports:
+    - port: 1433
+      targetPort: 1433


Reference the target port via name (see above).

Done. Thanks

unguiculus · 2018-01-29T21:14:14Z

/ok-to-test

thomasliddledba · 2018-01-29T22:23:33Z

@unguiculus - Thanks again - also moved to stable.

unguiculus · 2018-01-30T05:03:53Z

stable/mssql-linux/README.md

 ```console
-$ helm repo add incubator http://storage.googleapis.com/kubernetes-charts-incubator
-$ helm install --name mymssql incubator/mssql-linux --set acceptEula.value=Y --set edition.value=Developer
+$ helm repo add stable https://kubernetes-charts.storage.googleapis.com/


No need to add the stable repo.

Fixed. Thanks

unguiculus · 2018-01-30T05:04:50Z

/retest

unguiculus · 2018-02-02T12:21:44Z

/lgtm

k8s-ci-robot · 2018-02-02T12:21:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thomasliddledba, unguiculus

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

Details

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [unguiculus]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

Create SQL Server 2017 Linux HELM Chart

77353cd

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 5, 2018

thomasliddledba added 3 commits January 5, 2018 17:16

Fixed spaces for ci/circleci

a9197de

Fixed spaces for ci/circleci #2

cd8f11c

Fixed spaces for ci/circleci #3

6dff973

k8s-ci-robot assigned foxish Jan 5, 2018

thomasliddledba added 2 commits January 6, 2018 07:25

Added secret yaml to deployment for SA Password

8d2b290

Updated the chart to force user to specify EULA to Y for license terms

bfa7255

thomasliddledba mentioned this pull request Jan 6, 2018

added sql server on linux #1733

Closed

thomasliddledba added 2 commits January 6, 2018 09:50

Updated README.md to reflect commandline option to accept EULA

84f1f85

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

973eb15

…sqldevelop

thomasliddledba changed the title ~~Create SQL Server 2017 Linux HELM Chart~~ SQL Server 2017 Linux HELM Chart Jan 10, 2018

foxish reviewed Jan 11, 2018

View reviewed changes

k8s-ci-robot requested a review from lachie83 January 11, 2018 13:38

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 Jan 11, 2018

thomasliddledba added 2 commits January 11, 2018 19:42

Updated README.md and secret.yaml for sa_password generation

2ea47a7

Fixed formatting error in NOTES.txt

dfed42a

unguiculus suggested changes Jan 15, 2018

View reviewed changes

unguiculus self-assigned this Jan 15, 2018

thomasliddledba added 3 commits January 15, 2018 07:30

Changed apiVersion in deployments.yaml to apps/v1beta2

3a88881

Allowed ability to override sapassword and change apiVersion in deplo…

4891053

…yment.yaml

ci/circleci fixes

d078791

foxish assigned lachie83 and unassigned foxish Jan 24, 2018

unguiculus suggested changes Jan 27, 2018

View reviewed changes

Updated release version, README.md, and hard-coded ports

8008cc0

unguiculus suggested changes Jan 29, 2018

View reviewed changes

Added port name, configured probes, moved to stable

4afb6b8

unguiculus suggested changes Jan 30, 2018

View reviewed changes

Updated README.md

247a852

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2018

unguiculus approved these changes Feb 2, 2018

View reviewed changes

k8s-ci-robot merged commit 0c1830e into helm:master Feb 2, 2018

thomasliddledba deleted the mssqldevelop branch February 2, 2018 16:27

mattfarina unassigned lachie83 Jul 19, 2018

Conversation

thomasliddledba commented Jan 5, 2018

Uh oh!

thomasliddledba commented Jan 5, 2018

Uh oh!

thomasliddledba commented Jan 5, 2018

Uh oh!

thomasliddledba commented Jan 6, 2018

Uh oh!

thomasliddledba commented Jan 10, 2018

Uh oh!

foxish left a comment

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!

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!

foxish commented Jan 11, 2018

Uh oh!

thomasliddledba commented Jan 15, 2018

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!

thomasliddledba commented Jan 17, 2018

Uh oh!

thomasliddledba commented Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomasliddledba commented Jan 24, 2018

Uh oh!

unguiculus left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasliddledba commented Jan 24, 2018 •

edited

Loading