fix(cli): restore objects with I prefix fails

[NickIAm]

Fixes #2945
The tryToConvertToPath would return an error when given an ID with an I prefix. Call object.parseID instead of index.parseID Also set the default for snapshot-time as that was missing. In my testing this seems to fix the issue

[kakaroto]

I don't know if my PR approval means anything, but this looks good and matches my suggested fix from #2945

[NickIAm]

Any chance of this getting merged? @jkowalski

[kakaroto]

@jkowalski I don't want to sound ungrateful for the work you're doing on this FOSS project, but you have a pretty major regression that is affecting many people, and we're stuck on 0.11.3 because of it, and you have this simple one-liner PR that fixes the regression and you just made a release but still without merging this one.

I understand that you may be busy with life, but can we get an ETA on when you could get this merged and a new release published please? It's been over a year since the regression was introduced and it prevents restoring snapshots, which is pretty critical IMHO.

Thank you.

[jkowalski]

the code is currently failing a test, so that needs to be fixed before we can merge. We should add a regression test to ensure restore with I prefix is tested on a regular basis. Please run make to make sure tests pass.

[kakaroto]

the code is currently failing a test, so that needs to be fixed before we can merge. We should add a regression test to ensure restore with I prefix is tested on a regular basis. Please run make to make sure tests pass.

Thank you for responding to the PR!

It looks like the failure in unit test is "unexpected success" on TestRestoreByPathWithoutTarget.
I didn't try make locally, just checked the logs from github actions. I'm assuming it's because of the addition of .Default("latest") so that restoring by path can work without specifying a target, since the argument's help string says "default is latest" but the default wasn't actually set.

I'd say that (if my assumption on the failure is correct), the decision is yours:

• Should the unit test be fixed because a restore by path without target is now expected to work (and use latest as expected from the doc)?; or
• Should the default be removed and the argument help string modified to state that it's a required argument when restoring by path?

Thanks!

[NickIAm]

I've changed the unit test to reflect the --snapshot-time defaulting to "latest". It should now be passing.

I'm not really familiar with how unit tests work in general as it's not something I've ever had to deal with in the past. I didn't even notice that test was failing before @kakaroto pointed it out. Now I know what to look for going forward.

Thanks for your patience while I am learning about how all this works

[kakaroto]

I'm not really familiar with how unit tests work in general as it's not something I've ever had to deal with in the past. I didn't even notice that test was failing before @kakaroto pointed it out. Now I know what to look for going forward.

Thanks for doing the changes! The unit tests hadn't failed yet until I believe @jkowalski approved the github action to run (see now they're also marked as "awaiting approval") and he's the one who noticed them failing too :)

[jkowalski]

Something is wrong with the branch. Please rebase against latest master and repush.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.11%. Comparing base (cb455c6) to head (62900c4).
Report is 73 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3062      +/-   ##
==========================================
+ Coverage   75.86%   77.11%   +1.24%     
==========================================
  Files         470      476       +6     
  Lines       37301    28864    -8437     
==========================================
- Hits        28299    22258    -6041     
+ Misses       7071     4678    -2393     
+ Partials     1931     1928       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[NickIAm]

Sorry that got a bit messy. Looks better now

[kakaroto]

@jkowalski Hi, I just saw 0.16.0 and 0.16.1 releases and I'm wondering when this PR might get merged? It seems to keep falling through the cracks.
Bumping now so it can get a proper review/approval.
Thanks