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

Add VPC to k8s manifest (change default to persistent - true) #200

Closed
wants to merge 8 commits into from
Closed

Add VPC to k8s manifest (change default to persistent - true) #200

wants to merge 8 commits into from

Conversation

zamai
Copy link

@zamai zamai commented Sep 21, 2023

Pull Request

The plain k8s manifest (not the HELM version), does not have PVC for the kube stateful set.
This PR adds it for the next developer who wants to grab manifest for working meilisearch without using helm.

Related issue

Related issue:, #159

What does this PR do?

  • add's missing PVC to the k8s manifest

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@zamai
Copy link
Author

zamai commented Oct 5, 2023

@meilisearch sync-manifest

@brunoocasali
Copy link
Member

Hey @zamai can you check the files changed? It is empty :)

@zamai zamai changed the title Add VPC to k8s manifest Add VPC to k8s manifest (change default to persistent - true) Oct 12, 2023
@zamai
Copy link
Author

zamai commented Oct 12, 2023

@brunoocasali I figured that manifest is generated from helm charts, and I saw that persistence is turned off by default.

I suggest switching it to true by default, so that production scenario (where you defiantly want your search data to persist after pod restart) is the default.

@brunoocasali
Copy link
Member

Hi @zamai can you update the chart version to v0.4.0? Since you changed a default configuration the chart linters are complaining about that. We can merge it after that!

@zamai
Copy link
Author

zamai commented Nov 21, 2023

@meilisearch sync-manifest

@zamai
Copy link
Author

zamai commented Nov 21, 2023

@brunoocasali updated the PR

@brunoocasali
Copy link
Member

I can't figure out why the CI does not run 😬

Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

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

LGTM!

bors merge

meili-bors bot added a commit that referenced this pull request Nov 22, 2023
200: Add VPC to k8s manifest (change default to persistent - true) r=brunoocasali a=zamai

# Pull Request
The plain k8s manifest (not the HELM version), does not have PVC for the kube stateful set. 
This PR adds it for the next developer who wants to grab manifest for working meilisearch without using helm. 


## Related issue
Related issue:, #159 

## What does this PR do?
- add's missing PVC to the k8s manifest 

## PR checklist
Please check if your PR fulfills the following requirements:
- [ ] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
- [x] Have you read the contributing guidelines?
- [x] Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!


Co-authored-by: Alex Zamai <[email protected]>
Co-authored-by: Bruno Casali <[email protected]>
Co-authored-by: zamai <[email protected]>
@brunoocasali brunoocasali added enhancement New feature or request breaking-change The related changes are breaking for the users labels Nov 22, 2023
Copy link
Contributor

meili-bors bot commented Nov 22, 2023

Build failed:

  • helm-tests

@brunoocasali brunoocasali self-requested a review December 11, 2023 02:50
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

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

Hey @zamai can you help me with my doubts?

@@ -113,7 +113,7 @@ ingress:

persistence:
# -- Enable persistence using PVC
enabled: false
enabled: true
Copy link
Member

Choose a reason for hiding this comment

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

I don't know why I noticed that earlier...

You changed this to true but the PR description only mentions that the idea is to provide the PVC for the plain k8s version. Why this have to change then?

Copy link
Author

Choose a reason for hiding this comment

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

Initially I thought that plain k8s version is separate from helm charts, but when I saw that sync-manifest command generates k8s yaml files bases on charts values, so I updated this value to be true. I think it's a sensible default - having persistent storage for the index of your search cluster.

@@ -2,7 +2,7 @@ apiVersion: v1
appVersion: "v1.5.0"
description: A Helm chart for the Meilisearch search engine
name: meilisearch
version: 0.3.0
version: 0.4.0
Copy link
Member

Choose a reason for hiding this comment

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

Based on the previous comment, I guess this change could be reverted as well.

@zamai zamai closed this by deleting the head repository Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change The related changes are breaking for the users enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants