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 support for archive snapshot type #173

Merged
merged 3 commits into from
Mar 19, 2024

Conversation

okuuva
Copy link

@okuuva okuuva commented Feb 23, 2024

Fixes vmware-tanzu/velero#7371.

Signed-off-by: Oula Kuuva [email protected]

@okuuva okuuva force-pushed the support-archive-snapshots branch 2 times, most recently from 9733336 to ed76a6c Compare February 23, 2024 12:21
@okuuva okuuva force-pushed the support-archive-snapshots branch 2 times, most recently from ba70229 to b70c48f Compare February 23, 2024 12:43
@codecov-commenter
Copy link

codecov-commenter commented Feb 25, 2024

Codecov Report

Attention: Patch coverage is 35.29412% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 29.73%. Comparing base (a450b02) to head (bfe0ef1).

Files Patch % Lines
velero-plugin-for-gcp/volume_snapshotter.go 35.29% 10 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #173      +/-   ##
==========================================
+ Coverage   29.15%   29.73%   +0.58%     
==========================================
  Files           3        3              
  Lines         518      528      +10     
==========================================
+ Hits          151      157       +6     
- Misses        355      359       +4     
  Partials       12       12              

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

@tuusberg
Copy link

@okuuva I just opened a draft PR in this repo that solves the same problem and saw yours :) Great job!
I ran into an issue when I was working on my PR: Snapshot type in Google SDK v0.63.0 does not have SnapshotType property. I solved it by upgrading the SDK to the latest version (v0.167.0) and also bumped Go version, which, most probably deserves a separate PR.

@blackpiglet
Copy link
Collaborator

@okuuva
Thanks for the contribution.
The PR misses using the added snapshot type to create the snapshot.

Two places may need the snapshot type.

gceSnap := compute.Snapshot{
Name: snapshotName,
Description: getSnapshotTags(tags, disk.Description, b.log),
SourceDisk: disk.SelfLink,
}

gceSnap := compute.Snapshot{
Name: snapshotName,
Description: getSnapshotTags(tags, disk.Description, b.log),
SourceDisk: disk.SelfLink,
}

@okuuva
Copy link
Author

okuuva commented Feb 26, 2024

@okuuva I just opened a draft PR in this repo that solves the same problem and saw yours :) Great job!

Thanks!

I ran into an issue when I was working on my PR: Snapshot type in Google SDK v0.63.0 does not have SnapshotType property. I solved it by upgrading the SDK to the latest version (v0.167.0) and also bumped Go version, which, most probably deserves a separate PR.

Hmmh, where did you get the v0.63.0? To me it seems like it's currently sitting at v0.143.0 which isn't the latest but does include the SnapshotType property.

I personally try to separate version bumps from functional changes unless they're absolutely necessary. While bumping both the SDK version and Go version are probably a good idea I'd do those in separate PR(s).

@okuuva
Copy link
Author

okuuva commented Feb 26, 2024

@okuuva Thanks for the contribution. The PR misses using the added snapshot type to create the snapshot.

Two places may need the snapshot type.

gceSnap := compute.Snapshot{
Name: snapshotName,
Description: getSnapshotTags(tags, disk.Description, b.log),
SourceDisk: disk.SelfLink,
}

gceSnap := compute.Snapshot{
Name: snapshotName,
Description: getSnapshotTags(tags, disk.Description, b.log),
SourceDisk: disk.SelfLink,
}

...well, that was an awkward oversight from my part 😅 Fixed it now, thanks!

@tuusberg
Copy link

@okuuva nevermind, it seems my fork was outdated, it's 0.143.0 in the main branch and it would work without any modification.

@blackpiglet
Copy link
Collaborator

@okuuva
Thanks for your contribution. This PR looks good. Just delete the changelog file, then we can merge this PR.

Oula Kuuva added 2 commits February 27, 2024 09:12
How I managed to mess it up and not run gofmt, I don't know.

Signed-off-by: Oula Kuuva <[email protected]>
Oopsie daisie.

Signed-off-by: Oula Kuuva <[email protected]>
@blackpiglet blackpiglet merged commit 07c222c into vmware-tanzu:main Mar 19, 2024
2 checks passed
@gokulkrishnaa
Copy link

@blackpiglet what release version can we expect this fix to be a part of?

@blackpiglet
Copy link
Collaborator

This plugin will be shipped with Velero v1.14.0; the version should be v1.10.0.

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.

(GCP) Users should be able to select a snapshot type (archive or standard) when creating backups
6 participants