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

added option to use emptyDir as additional mount #512

Merged
merged 5 commits into from
Aug 16, 2023

Conversation

mdarii
Copy link
Contributor

@mdarii mdarii commented May 12, 2023

related to issue: #484

This commit would allow to add additional volumes as emptyDir, this is necessary if readOnlyRootFilesystem=true

@idanl21
Copy link
Collaborator

idanl21 commented May 23, 2023

@mdarii
Please take a look

Copy link
Collaborator

@swoehrl-mw swoehrl-mw left a comment

Choose a reason for hiding this comment

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

Hi @mdarii.
Thank you for your contribution.

I have some comments:

  • Please add a unittest for the new functionality to verify it works (you can extend the test in controllers/cluster_test.go by adding an additionalVolume to the spec and verifying the volume(mount) is part of the created STS).
  • Extend the section on "Additional Volumes" in the userguide
  • Extend the CRD reference (docs/designs/crd.md)

@mdarii
Copy link
Contributor Author

mdarii commented May 26, 2023

Hi @swoehrl-mw , I've updated my PR.

@swoehrl-mw
Copy link
Collaborator

@mdarii Code looks good, please check why the tests fail and fix if necessary.

@mdarii
Copy link
Contributor Author

mdarii commented May 26, 2023

@swoehrl-mw I'll look into it, but most probably next week

@mdarii
Copy link
Contributor Author

mdarii commented Jun 5, 2023

@swoehrl-mw looks like I've found typo, can you please retrigger pipeline?

@swoehrl-mw
Copy link
Collaborator

@mdarii Could you please merge in the current main? There is a fix that should deal with the failing functionaltests check so we can then hopefully merge this PR.

@idanl21
Copy link
Collaborator

idanl21 commented Jul 25, 2023

@mdarii
Hey , do you plan to continue that PR ?

@mdarii
Copy link
Contributor Author

mdarii commented Aug 9, 2023

@mdarii Hey , do you plan to continue that PR ?

yes, I'll try to come back to it somehow this week

@mdarii
Copy link
Contributor Author

mdarii commented Aug 12, 2023

@swoehrl-mw could you trigger pipeline please, I've synced repo with master

Copy link
Collaborator

@swoehrl-mw swoehrl-mw left a comment

Choose a reason for hiding this comment

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

LGTM

@swoehrl-mw swoehrl-mw merged commit ac78faa into opensearch-project:main Aug 16, 2023
6 checks passed
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.

3 participants