Skip to content
This repository was archived by the owner on Feb 22, 2022. It is now read-only.

Conversation

@shashiranjan84
Copy link
Collaborator

  • Chart deploys Kong and its backing datastore either Cassandra or
    PostgreSql(default)
  • Uses official Kong Docker image

Note: Opening this new PR instead of #3031.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 26, 2017
@shashiranjan84
Copy link
Collaborator Author

/assign @unguiculus

@shashiranjan84
Copy link
Collaborator Author

I have updated the the labels and made few more changes as advised in best practices guide.

Copy link
Member

@unguiculus unguiculus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I did an initial review.

Copy link
Member

Choose a reason for hiding this comment

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

Add appVersion field.

Copy link
Member

Choose a reason for hiding this comment

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

Add release label to selector.

Copy link
Member

Choose a reason for hiding this comment

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

Add release label to selector.

Copy link
Member

Choose a reason for hiding this comment

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

Add release label to selector.

Copy link
Member

Choose a reason for hiding this comment

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

Add release label to selector.

Copy link
Member

Choose a reason for hiding this comment

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

Only app and release labels should go here.

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like you have real components in your chart. There is only a single deployment. Usually, components are used to identify multiple deployments in a chart and then are added to label selectors. This is not the case here. Do they server any special purpose?

Copy link
Member

Choose a reason for hiding this comment

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

kong.database.type is not set anywhere but mentioned here.

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't make sense to reproduce the whole Cassandra and Postgres values files here.

Copy link
Member

Choose a reason for hiding this comment

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

You should not added a second field for the DB password. There is already postgresql.postgresPassword. Also, by default, a random password is generated. Let's keep it that way.

@shashiranjan84
Copy link
Collaborator Author

@unguiculus thanks for review. I will make the changes you advised. Please expect some delay as I am travelling and have limited internet connectivity.

- Chart deploys Kong and its backing datastore either Cassandra or
  PostgreSql(default)
- Uses official [Kong](https://hub.docker.com/_/kong/) Docker image
@shashiranjan84
Copy link
Collaborator Author

@unguiculus please review.

Copy link
Member

@unguiculus unguiculus left a comment

Choose a reason for hiding this comment

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

Please don't squash commits. They will be squashed upon merge anyways.


## Prerequisites

- Kubernetes 1.6+ with Beta APIs enabled.
Copy link
Member

Choose a reason for hiding this comment

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

1.8+

We offically support the current and previous minor versions.

@@ -0,0 +1,88 @@
apiVersion: extensions/v1beta1
Copy link
Member

Choose a reason for hiding this comment

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

Use apps/v1beta2

enabled: true
postgresUser: kong
postgresPassword: kong
postgresDatabase: kong
Copy link
Member

Choose a reason for hiding this comment

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

Comment this out so the password is auto-generated by default.

@unguiculus
Copy link
Member

/ok-to-test

@k8s-ci-robot 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 13, 2018
replicas: {{ .Values.replicaCount }}
selector:
matchLabels:
app: {{ template "kong.name" . }}
Copy link
Member

Choose a reason for hiding this comment

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

Add release label to selector.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@shashiranjan84
Copy link
Collaborator Author

@unguiculus are we still waiting on something?

@shashiranjan84
Copy link
Collaborator Author

ping @unguiculus

- name: {{ template "kong.name" . }}-migrations
image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
imagePullPolicy: {{ .Values.image.pullPolicy }}
env:
Copy link
Member

@unguiculus unguiculus Jan 27, 2018

Choose a reason for hiding this comment

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

For consistency, don't indent this list.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

makes sense, @unguiculus I fixed it.

@unguiculus
Copy link
Member

/ok-to-test

@unguiculus
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2018
@k8s-ci-robot k8s-ci-robot merged commit 356c348 into helm:master Jan 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants