Skip to content
This repository has been archived by the owner on May 16, 2023. It is now read-only.

bump es version, remove default storageClassName #94

Merged
merged 7 commits into from
Apr 17, 2019

Conversation

kimxogus
Copy link
Contributor

This pr

  1. Bump default es version to 6.7.1
  2. Removes standard storageClassName by default as this value will be default storage class if empty(you don't need to specify this). In aws environment, default storage class is generally gp2, so volumeClaimTemplate should be always overridden.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@kimxogus
Copy link
Contributor Author

should be 7.0.0?

@z0lope0z
Copy link

👍 trying to helm install on new AWS EKS cluster and getting the error

persistentvolume-controller storageclass.storage.k8s.io "standard" not found

Copy link
Contributor

@Crazybus Crazybus left a comment

Choose a reason for hiding this comment

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

Apart from removing the version bump and a slight readme tweak this LGTM!

@@ -4,8 +4,8 @@ maintainers:
- email: helm-charts@elastic.co
name: Elastic
name: elasticsearch
version: 6.6.2-alpha1
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this version bump please. All versions of the charts are bumped and released at the same time (just like with all of the elastic stack). I went into more details in this PR if you are interested in the reasons why: #40 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted chart version bump

@@ -71,7 +71,6 @@ networkHost: "0.0.0.0"

volumeClaimTemplate:
accessModes: [ "ReadWriteOnce" ]
storageClassName: "standard"
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! This is much better. Can you also update the defaults in the readme to match https://github.com/elastic/helm-charts/tree/master/elasticsearch#configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated readme

@Crazybus
Copy link
Contributor

jenkins test this please

@@ -5,7 +5,7 @@ maintainers:
name: Elastic
name: elasticsearch
version: 6.6.2-alpha1
appVersion: 6.6.2
appVersion: 6.7.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Something went wrong with the revert as the version bump is still here.

@@ -55,14 +55,14 @@ If you currently have a cluster deployed with the [helm/charts stable](https://g
| `extraInitContainers` | Additional init containers to be passed to the `tpl` function | |
| `secretMounts` | Allows you easily mount a secret as a file inside the statefulset. Useful for mounting certificates and other secrets. See [values.yaml](./values.yaml) for an example | `{}` |
| `image` | The Elasticsearch docker image | `docker.elastic.co/elasticsearch/elasticsearch` |
| `imageTag` | The Elasticsearch docker image tag | `6.6.2` |
| `imageTag` | The Elasticsearch docker image tag | `6.7.1` |
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be reverted too.

@@ -43,7 +43,7 @@ secretMounts: []
# path: /usr/share/elasticsearch/config/certs

image: "docker.elastic.co/elasticsearch/elasticsearch"
imageTag: "6.6.2"
imageTag: "6.7.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

This one too

Copy link
Contributor

@Crazybus Crazybus left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks so much for adding this in. It's going to make it so much easier for users on different Kubernetes providers.

@Crazybus Crazybus merged commit c5289d5 into elastic:master Apr 17, 2019
@kimxogus kimxogus deleted the es/update-values branch April 17, 2019 15:52
Crazybus added a commit that referenced this pull request May 1, 2019
This test will install the latest chart release and then attempt to
upgrade to what is currently in git. This is designed to test two
things.

1. That any changes do not happen to the statefulset that isn't allowed
by Kubernetes. An example of this happening is #94 where the change
was not caught as part of pull request testing but was luckily caught
during manual testing before the release.
2. That a rolling upgrade actually works and results in a healthy
cluster at the end. The test will always override the
"terminationGracePeriod" to make sure that a rolling upgrade is always
done even if there are no changes.

Kubectl and helm have also been updated to the latest releases. Oddly
enough there was a breaking change in the mock value used for release
names where it is now lowercased for some reason.

The kubectl upgraded was needed to get the handy timeout flag for the
`kubectl rollout status`command to make sure that jobs don't hang
forever if something does actually go wrong. This command is needed
because the `helm --wait` flag only works `apps/v1` resources. See
helm/helm#3173 for more information.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants