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

[incubator/monochart] Add new chart #155

Merged
merged 31 commits into from
Aug 29, 2018
Merged

[incubator/monochart] Add new chart #155

merged 31 commits into from
Aug 29, 2018

Conversation

alebabai
Copy link
Contributor

@alebabai alebabai commented Aug 10, 2018

what

references

@@ -0,0 +1,10 @@
apiVersion: v1
description: Helm Chart
Copy link
Member

Choose a reason for hiding this comment

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

A declarative helm chart for deploying common types of services on kubernetes

same "printed page" as the copyright notice for easier
identification within third-party archives.

Copyright [yyyy] [name of copyright owner]
Copy link
Member

Choose a reason for hiding this comment

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

Fix

@@ -0,0 +1,3 @@
# Helm Chart Scaffolding

Use this chart when creating new charts to make it easy to get up and running quickly.
Copy link
Member

Choose a reason for hiding this comment

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

Use the description from above

@@ -0,0 +1,3 @@
# Helm Chart Scaffolding
Copy link
Member

Choose a reason for hiding this comment

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

Rename to Monochart

- configMapRef:
name: {{ include "common.fullname" . }}
env:
{{- range $name, $value := .Values.env.open }}
Copy link
Member

@osterman osterman Aug 12, 2018

Choose a reason for hiding this comment

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

We need to pick one of these ways of exporting envs, because currently we export all envs twice.

Maybe we use this values format?

env:
  configMap:
    USER: bar
  secrets:
    PASSWORD: secret
  inline:
     DEBUG: true

Copy link
Member

@osterman osterman Aug 14, 2018

Choose a reason for hiding this comment

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

Or maybe we should use this format, since we already have sections for job, deployment, service, etc... we should do the same for configMap and secret resource types.

configMap:
  env:
    USER: bar
  files:
     "/etc/test.conf": "foo"

secrets:
  env:
    PASS: secret
  files:
     "/etc/ssl/test.crt": |-
       ...

# Inline env
env:
  USER: test

@osterman osterman changed the title Incubator - Add monochart [monochart] Add new chart Aug 12, 2018
config:
# name: value
env:
open:
Copy link
Member

Choose a reason for hiding this comment

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

See above. I think we should have configMap, secrets and inline types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@osterman good point, but I have some thoughts about this.
Configmap usually contains properties like a.b.c: value and this is not really an environment variable (it's looks like the set of config-files more)

Copy link
Member

Choose a reason for hiding this comment

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

Yea, what about dropping the env top-level convention and instead doing:

configMaps:
  env:
    FOO: bar
   files:
     "/etc/blah.conf": "asd"
secrets:
 # same....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeap, this variant looks good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed a PR to common chart with changes required to render env from configmaps.
But do we need dependency on common chart or monochart should have all required templates?

@@ -0,0 +1,108 @@
replicaCount: 1
image:
repository: nginx
Copy link
Member

Choose a reason for hiding this comment

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

Let's leave this blank and error if not set.

replicaCount: 1
image:
repository: nginx
tag: 0.1.0
Copy link
Member

Choose a reason for hiding this comment

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

Let's leave this blank and error if not set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@osterman i'll should revert these changes, because it breaks CI.

# annotations:
## Read more about kube2iam to provide access to s3 https://github.com/jtblin/kube2iam
# iam.amazonaws.com/role: role-arn
service:
Copy link
Member

Choose a reason for hiding this comment

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

Add enabled flag

cpu: 80m
memory: 64Mi
persistence:
Enabled: false
Copy link
Member

Choose a reason for hiding this comment

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

let's lowercase first character

# annotations:
## Read more about kube2iam to provide access to s3 https://github.com/jtblin/kube2iam
# iam.amazonaws.com/role: role-arn
statefulset:
Copy link
Member

Choose a reason for hiding this comment

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

Add enabled flag

ENV_NAME: ENV_VALUE
secret:
SECRET_ENV_NAME: SECRET_ENV_VALUE
deployment:
Copy link
Member

Choose a reason for hiding this comment

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

Add enabled flag

# annotations:
## Read more about kube2iam to provide access to s3 https://github.com/jtblin/kube2iam
# iam.amazonaws.com/role: role-arn
daemonset:
Copy link
Member

Choose a reason for hiding this comment

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

Add enabled flag

# annotations:
## Read more about kube2iam to provide access to s3 https://github.com/jtblin/kube2iam
# iam.amazonaws.com/role: role-arn
job:
Copy link
Member

Choose a reason for hiding this comment

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

Add enabled flag

emptyDir: {}
{{- end -}}

- name: config
Copy link
Member

Choose a reason for hiding this comment

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

We need to generalize this configuration and give it a proper section in values.yaml:

e.g.

configFiles:
  "/etc/test.conf": |-
    [default]
    debug=true

- key: special.level
path: keys

- name: secret
Copy link
Member

Choose a reason for hiding this comment

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

We need to generalize this too.

@osterman osterman changed the title [monochart] Add new chart [incubator/monochart] Add new chart Aug 14, 2018
@osterman osterman requested a review from aknysh August 14, 2018 16:35
version: 0.1.0
appVersion: 0.1.0
home: https://github.com/cloudposse/charts/tree/master/incubator/monochart
icon: https://github.com/cloudposse/charts/tree/master/incubator/monochart/logo.png
Copy link
Member

Choose a reason for hiding this comment

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

wrong logo URL

@@ -0,0 +1,3 @@
# Monochart

A declarative helm chart for deploying common types of services on kubernetes.
Copy link
Member

Choose a reason for hiding this comment

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

Capitalize Kubernetes

@@ -0,0 +1,10 @@
apiVersion: v1
description: A declarative helm chart for deploying common types of services on kubernetes
Copy link
Member

Choose a reason for hiding this comment

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

Capitalize Kubernetes

@alebabai alebabai added the wip label Aug 28, 2018
@alebabai alebabai removed the wip label Aug 29, 2018
@alebabai alebabai self-assigned this Aug 29, 2018
args: {}
ports:
- name: http
containerPort: 8080
Copy link
Member

Choose a reason for hiding this comment

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

Parameterize?

args: {}
ports:
- name: http
containerPort: 8080
Copy link
Member

Choose a reason for hiding this comment

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

Parameterize?

Copy link
Contributor Author

@alebabai alebabai Aug 29, 2018

Choose a reason for hiding this comment

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

In newer version of helm (2.9.1 and higher) when your create chart via helm create chart_name it has some changes in service template and deployments ports and i think it really reasonable changes. Service have only one parameterized port - external one, and containers port is hardcoded with assigned name.
I'll explain.
Container port depends on docker image that you use therefore it is unlikely that it will ever be changed - so it can be hardcoded directly in deployment, but it shoud have a name http in this case. External port of the service is some kind of interface so it shoud be configurable.

Copy link
Member

Choose a reason for hiding this comment

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

I think that makes sense for a custom built chart for a single purpose. But this is a universal chart so I think it’s better we parameteize it for more interoperability. Eg node apps default on 3000.

@@ -0,0 +1,4 @@
dependencies:
- name: common
Copy link
Member

Choose a reason for hiding this comment

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

do we still need this if we have _helpers.tpl as well?

# - "docker-secret-1"
# - "docker-secret-2"

configMap:
Copy link
Member

Choose a reason for hiding this comment

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

Add support for labels so that we can use with Grafana

args: {}
ports:
- name: http
containerPort: 8080
Copy link
Member

Choose a reason for hiding this comment

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

I think that makes sense for a custom built chart for a single purpose. But this is a universal chart so I think it’s better we parameteize it for more interoperability. Eg node apps default on 3000.

pod:
## Pod annotations
annotations: {}
## Read more about kube2iam to provide access to s3 https://github.com/jtblin/kube2iam
Copy link
Member

Choose a reason for hiding this comment

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

do we need a reference to kube2iam here since we mostly use kiam?

@osterman osterman merged commit 4248d1c into master Aug 29, 2018
@osterman
Copy link
Member

Let's move all changes/fixes/enhancements to a new PR.

@aknysh aknysh deleted the feature/monochart branch January 11, 2019 03:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants