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

feat: add "additionalVolumes" for lvmd and node ds #814

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

ryanfaircloth
Copy link
Contributor

Additional volumes simplifies configuration for init pods in the data sets by allowing additional volumes without changing the required topolvm volumes

@pluser
Copy link
Contributor

pluser commented Jan 15, 2024

Is there a commit (67bc915) unrelated to this PR in the mix?

@ryanfaircloth
Copy link
Contributor Author

ryanfaircloth commented Jan 15, 2024 via email

@ryanfaircloth
Copy link
Contributor Author

Updated I forgot to set he default in values.yaml

@ryanfaircloth ryanfaircloth force-pushed the chart-additional-vols branch 2 times, most recently from e451c28 to 3969894 Compare January 15, 2024 13:44
charts/topolvm/templates/lvmd/daemonset.yaml Outdated Show resolved Hide resolved
charts/topolvm/templates/node/daemonset.yaml Outdated Show resolved Hide resolved
charts/topolvm/values.yaml Outdated Show resolved Hide resolved
charts/topolvm/values.yaml Outdated Show resolved Hide resolved
charts/topolvm/values.yaml Outdated Show resolved Hide resolved
charts/topolvm/values.yaml Outdated Show resolved Hide resolved
charts/topolvm/values.yaml Outdated Show resolved Hide resolved
charts/topolvm/templates/node/daemonset.yaml Outdated Show resolved Hide resolved
charts/topolvm/templates/lvmd/daemonset.yaml Outdated Show resolved Hide resolved
@ryanfaircloth ryanfaircloth force-pushed the chart-additional-vols branch 4 times, most recently from b4b0385 to fe3d0c4 Compare January 16, 2024 14:46
@ryanfaircloth
Copy link
Contributor Author

I believe all requested changes are complete, constructive feedback thank you

charts/topolvm/values.yaml Outdated Show resolved Hide resolved
charts/topolvm/values.yaml Outdated Show resolved Hide resolved
charts/topolvm/values.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@toshipp toshipp left a comment

Choose a reason for hiding this comment

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

@ryanfaircloth
Thank you for your contribution! LGTM.

@pluser
Copy link
Contributor

pluser commented Jan 19, 2024

Could you please clean up the commit message?
It looks like you are reverting to commits that have not been merged into the master branch.
Once that is resolved, this PR looks fine.

@toshipp toshipp self-requested a review January 22, 2024 01:16
@toshipp
Copy link
Contributor

toshipp commented Jan 22, 2024

@ryanfaircloth
Could you check @pluser's message?

@ryanfaircloth
Copy link
Contributor Author

Could you please clean up the commit message? It looks like you are reverting to commits that have not been merged into the master branch. Once that is resolved, this PR looks fine.

Can you show me what you are referring to happy to fix, what I see as my commit message here
image

@toshipp
Copy link
Contributor

toshipp commented Jan 23, 2024

@ryanfaircloth
Did you check this URL?
03cbb92

What I see is
image

@ryanfaircloth
Copy link
Contributor Author

@ryanfaircloth Did you check this URL? 03cbb92

What I see is image

Fixed it was a silly issue on my side I used github desktop to amend the message and it was cutting off after the first signoff for future reference I know to watch for that

toshipp
toshipp previously approved these changes Jan 25, 2024
@toshipp
Copy link
Contributor

toshipp commented Jan 25, 2024

@ryanfaircloth
Sorry, I forgot we should run helm-docs to update the chart readme.
Could you do that?
The instructions are following.

make install-helm-docs
./bin/helm-docs

@ryanfaircloth
Copy link
Contributor Author

@toshipp README updated

@toshipp
Copy link
Contributor

toshipp commented Jan 30, 2024

@ryanfaircloth
Could you fix a failure of chart lint?
https://github.com/topolvm/topolvm/pull/814/files#diff-80f31301c246264c6d93fb70fe8700bef090f82dec686d430e06ed2ac7db59d9R182

Also, please rebase it on the main branch to remove the merge commit.

I think there are no other blockers to merge this.

@ryanfaircloth ryanfaircloth force-pushed the chart-additional-vols branch 3 times, most recently from 814f58f to cb62b33 Compare January 31, 2024 11:28
@ryanfaircloth
Copy link
Contributor Author

@toshipp fixed spaces and rebased

@toshipp
Copy link
Contributor

toshipp commented Feb 1, 2024

@ryanfaircloth
I confirmed the lint succeeded.
BTW, other commits not related to PR are introduced. Could you omit them? If it is difficult for you, can I do it instead?

@ryanfaircloth
Copy link
Contributor Author

ryanfaircloth commented Feb 1, 2024 via email

Additional volumes simplifies configuration for init pods in the data
sets by allowing additional volumes without changing the required
topolvm volumes

Signed-off-by: Ryan Faircloth <ryan@dss-i.com>
@pluser pluser merged commit a2a4aae into topolvm:main Feb 2, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants