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

Create new ItemBlockAction (IBA) plugin type #8026

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

sseago
Copy link
Collaborator

@sseago sseago commented Jul 18, 2024

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

First PR for #7900 (but not the complete feature)

Please indicate you've done the following:

  • [x ] Accepted the DCO. Commits without the DCO will delay acceptance.
  • [x ] Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • [x ] Updated the corresponding documentation in site/content/docs/main.

return nil, err
}

backupItemAction, ok := plugin.(ibav1.ItemBlockAction)
Copy link
Contributor

Choose a reason for hiding this comment

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

backup item action is a good name here?

itemBlockAction would be better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mateusoliveira43 oops. Copy-and-paste error. Will fix.

Copy link

codecov bot commented Jul 18, 2024

Codecov Report

Attention: Patch coverage is 36.59794% with 123 lines in your changes missing coverage. Please review.

Project coverage is 58.74%. Comparing base (d9ca147) to head (ba9c109).

Files Patch % Lines
...ork/itemblockaction/v1/item_block_action_client.go 0.00% 49 Missing ⚠️
...ork/itemblockaction/v1/item_block_action_server.go 47.27% 25 Missing and 4 partials ⚠️
pkg/plugin/framework/action_resolver.go 0.00% 18 Missing ⚠️
pkg/plugin/framework/server.go 0.00% 10 Missing ⚠️
...temblockaction/v1/restartable_item_block_action.go 77.14% 8 Missing ⚠️
.../framework/itemblockaction/v1/item_block_action.go 0.00% 5 Missing ⚠️
pkg/plugin/clientmgmt/manager.go 85.00% 2 Missing and 1 partial ⚠️
pkg/plugin/framework/common/plugin_kinds.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8026      +/-   ##
==========================================
- Coverage   58.88%   58.74%   -0.15%     
==========================================
  Files         351      355       +4     
  Lines       29367    29561     +194     
==========================================
+ Hits        17294    17365      +71     
- Misses      10639    10757     +118     
- Partials     1434     1439       +5     

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

@@ -77,6 +79,12 @@ type Manager interface {
// GetDeleteItemAction returns the delete item action plugin for name.
GetDeleteItemAction(name string) (velero.DeleteItemAction, error)

// GetItemBlockActions returns all v1 ItemBlock action plugins.
GetItemBlockActions() ([]ibav1.ItemBlockAction, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a standard, but would suggest changing name here to avoid confusion, since 2 functions have very similar name

for example, changing here to GetAllItemBlockActions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd rather not make this plugin inconsistent with every other plugin. Every other plugin type uses singular name to get one, and plural name to get all.

protoibav1 "github.com/vmware-tanzu/velero/pkg/plugin/generated/itemblockaction/v1"
)

// BackupItemActionPlugin is an implementation of go-plugin's Plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mateusoliveira43 yes -- will fix this one too

@sseago sseago force-pushed the itemblockaction branch 2 times, most recently from 6331d15 to f62eeb1 Compare July 23, 2024 13:17
@github-actions github-actions bot added the Area/Design Design Documents label Jul 23, 2024
@sseago sseago mentioned this pull request Jul 26, 2024
@blackpiglet
Copy link
Contributor

I only have one comment.
I couldn't find how the mock code was generated.
Could we evaluate whether there is a need to document it?

@sseago
Copy link
Collaborator Author

sseago commented Jul 31, 2024

@blackpiglet Mock generation is covered here: https://velero.io/docs/main/code-standards/#mocks

@shubham-pampattiwar shubham-pampattiwar merged commit 7811b9f into vmware-tanzu:main Jul 31, 2024
43 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants