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

Consul - Add consul tests and upgrade consul to 0.8.x #1126

Merged
merged 4 commits into from
Jun 8, 2017

Conversation

lachie83
Copy link
Contributor

  • Add app version tag
  • Update chart maintainers to github username
  • Add consul helm tests
  • Update documentation
  • Remove existing test.sh

Add app version tag
Update chart maintainers to github username
Add consul helm tests
Update documentation
Remove existing test.sh
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 18, 2017
@lachie83
Copy link
Contributor Author

cc @foxish hoping to add more testable charts

Copy link
Member

@prydonius prydonius left a comment

Choose a reason for hiding this comment

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

Awesome @lachie83, just have some small comments.

@@ -48,13 +47,15 @@ The following tables lists the configurable parameters of the consul chart and t
| `ui.enabled` | Enable Consul Web UI | `false` |
| `uiService.enabled` | Create dedicated Consul Web UI svc | `false` |
| `uiService.type` | Dedicate Consul Web UI svc type | `NodePort` |
| `test.image` | Test container image requires kubectl + bash (used for helm test) | `lachlanevenson/k8s-kubectl` |
Copy link
Member

Choose a reason for hiding this comment

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

should this be configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think this is fine. I've documented the prerequisites to cut your own container to run the tests if desired

2. Confirm consul cluster is healthy
$ kubectl exec {{ .Release.Name }}-consul-0 consul members --namespace={{ .Release.Namespace }} | grep server
2. Test cluster health using Helm test.
$ helm test {{ .Release.Namespace }}
Copy link
Member

Choose a reason for hiding this comment

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

should be .Release.Name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. Nice catch!

@lachie83
Copy link
Contributor Author

lachie83 commented Jun 2, 2017

Updated. I think this should be good to go.

@lachie83
Copy link
Contributor Author

lachie83 commented Jun 2, 2017

@k8s-bot pull-charts-e2e test this

@lachie83
Copy link
Contributor Author

lachie83 commented Jun 2, 2017

@k8s-bot pull-charts-e2e test this

@lachie83
Copy link
Contributor Author

lachie83 commented Jun 5, 2017

@prydonius can you please take another pass at this?

@lachie83 lachie83 dismissed prydonius’s stale review June 7, 2017 19:04

Address concerns

@prydonius prydonius added the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2017
Copy link
Member

@prydonius prydonius left a comment

Choose a reason for hiding this comment

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

sorry for the delay @lachie83, lgtm!

@prydonius prydonius merged commit ec52ae4 into helm:master Jun 8, 2017
yanns pushed a commit to yanns/charts that referenced this pull request Jul 28, 2017
* Bump consul version to 0.8.3
Add app version tag
Update chart maintainers to github username
Add consul helm tests
Update documentation
Remove existing test.sh

* Update helm test command in notes
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. code reviewed lgtm Indicates that a PR is ready to be merged. size/medium UX reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants