utkuozdemir / pv-migrate

CLI tool to easily migrate Kubernetes persistent volumes
Apache License 2.0
1.63k stars 82 forks source link

Remove helm rsync.fixPrivateKeyPerms option and change how ssh private key is mounted and used #252

Closed alex-vmw closed 1 year ago

alex-vmw commented 1 year ago

Remove helm rsync.fixPrivateKeyPerms option and change how ssh private key is mounted and used

Signed-off-by: Alex Romanenko <alex@vmware.com

Types of changes

Testing Done

Manually tested local and lbsvc usecases.

codecov-commenter commented 1 year ago

Codecov Report

Merging #252 (5d68d21) into master (44e5990) will not change coverage. The diff coverage is 100.00%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##           master     #252   +/-   ##
=======================================
  Coverage   69.56%   69.56%           
=======================================
  Files          23       23           
  Lines        1876     1876           
=======================================
  Hits         1305     1305           
  Misses        452      452           
  Partials      119      119           
Files Coverage Δ
strategy/lbsvc.go 85.24% <100.00%> (ø)
strategy/local.go 71.12% <100.00%> (ø)
strategy/svc.go 87.75% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

utkuozdemir commented 1 year ago

Looks cool, thank you for the work you put in. I like this solution. Pls see my comments, other than those it's all good.

alex-vmw commented 1 year ago

@utkuozdemir I have accepted your changes with the quotes, but the quotes actually broke ~ shell expansion, which broke the chmod command. :( I think we have 2 choices:

  1. Leave ~ outside of the quotes (as in chmod 400 ~/.ssh/"$privateKeyFilename").
  2. Use $HOME instead of ~ everywhere.
utkuozdemir commented 1 year ago

Hm, I'm wondering how the integration tests have passed... I updated the chart tgz in your branch. Integration tests should have failed.

alex-vmw commented 1 year ago

No idea why tests haven't failed :(, but the issue is there:

+ chmod 400 '~/.ssh/id_ed25519'
chmod: ~/.ssh/id_ed25519: No such file or directory
alex-vmw commented 1 year ago

I just tested a fix with chmod 400 ~/.ssh/"$privateKeyFilename" and it is working fine and can send an MR.

utkuozdemir commented 1 year ago

I like $HOME in quotes a bit better, feel free to submit a PR.

alex-vmw commented 1 year ago

OK, let me re-test with $HOME first. :)

alex-vmw commented 1 year ago

OK, $HOME is working (see below), so will submit the MR now.

+ mkdir -p /root/.ssh
+ chmod 700 /root/.ssh
+ cp -v /tmp/id_ed25519 /root/.ssh/
'/tmp/id_ed25519' -> '/root/.ssh/id_ed25519'
+ chmod 400 /root/.ssh/id_ed25519
utkuozdemir commented 1 year ago

Cool, please don't forget to do task update-chart.