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.

Sign up for GitHub

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

Jump to bottom

Add volume types filter in resource policies #6863

Merged
merged 1 commit into from
Oct 16, 2023
Merged

Add volume types filter in resource policies #6863

merged 1 commit into from
Oct 16, 2023

Conversation

qiuming-best
Copy link
Contributor

@qiuming-best qiuming-best commented Sep 25, 2023 •
edited
Loading

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #(issue)
#6482

Please indicate you've done the following:

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

@codecov
Copy link

codecov bot commented Sep 25, 2023 •
edited
Loading

Codecov Report

Merging #6863 (3617c27) into main (d3e5bb7) will increase coverage by 0.21%.
Report is 10 commits behind head on main.
The diff coverage is 98.31%.

@@            Coverage Diff             @@
##             main    #6863      +/-   ##
==========================================
+ Coverage   60.78%   61.00%   +0.21%     
==========================================
  Files         245      248       +3     
  Lines       26255    26492     +237     
==========================================
+ Hits        15960    16162     +202     
- Misses       9164     9192      +28     
- Partials     1131     1138       +7
Files Coverage Δ
internal/resourcepolicies/resource_policies.go 76.82% <100.00%> (+0.28%) ⬆️
internal/resourcepolicies/volume_resources.go 88.98% <100.00%> (+0.18%) ⬆️
...nal/resourcepolicies/volume_resources_validator.go 100.00% <ø> (ø)
...ternal/resourcepolicies/volume_types_conditions.go 98.28% <98.28%> (ø)

... and 11 files with indirect coverage changes

@qiuming-best qiuming-best force-pushed the filter-by-volumes-type branch 2 times, most recently from a6563e6 to 5139320 Compare September 25, 2023 03:42
@qiuming-best qiuming-best changed the title Add volume types filter in resource policies [WIP]Add volume types filter in resource policies Sep 25, 2023
@qiuming-best qiuming-best force-pushed the filter-by-volumes-type branch 3 times, most recently from c4eaaeb to 597d443 Compare September 26, 2023 04:04
@qiuming-best qiuming-best changed the title [WIP]Add volume types filter in resource policies Add volume types filter in resource policies Sep 26, 2023
@blackpiglet blackpiglet mentioned this pull request Sep 26, 2023
Closed
shubham-pampattiwar
shubham-pampattiwar reviewed Sep 26, 2023
View reviewed changes
Copy link
Collaborator

@shubham-pampattiwar shubham-pampattiwar left a comment

Choose a reason for hiding this comment

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

Added one comment, else lgtm!

internal/resourcepolicies/volume_types_conditions.go Outdated
return "gcePersistentDisk"
}
if vol.AWSElasticBlockStore != nil {
return "awsElasticBlockStore"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we make all the PV and Volume types as constants ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, modified

mateusoliveira43
mateusoliveira43 reviewed Sep 27, 2023
View reviewed changes
internal/resourcepolicies/volume_types_conditions.go Outdated
ConfigMap = "configMap"
Projected = "projected"
Ephemeral = "ephemeral"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

2 suggestions:

  • would be interesting to transform this in a type, like supportedVolumes, and change the return values of getVolumeTypeFromPV and getVolumeTypeFromVolume functions to it?
  • would be interesting to sort the volumes in alphabetical order? (and also in the getVolumeTypeFromPV and getVolumeTypeFromVolume functions bodies)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestions, I've updated

@draghuram
Copy link
Contributor

Is this going to be merged soon? See https://kubernetes.slack.com/archives/C6VCGP4MT/p1697461184984549 for a user question in this regard.

@sseago sseago merged commit 5ff5073 into vmware-tanzu:main Oct 16, 2023
blackpiglet pushed a commit to blackpiglet/velero that referenced this pull request Oct 19, 2023
@qiuming-best
Add volume types filter in resource policies (vmware-tanzu#6863)
4fdca20 
Signed-off-by: Ming Qiu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mateusoliveira43 mateusoliveira43 mateusoliveira43 approved these changes

@shubham-pampattiwar shubham-pampattiwar shubham-pampattiwar approved these changes

@kaovilai kaovilai kaovilai approved these changes

@sseago sseago sseago approved these changes

@reasonerjt reasonerjt Awaiting requested review from reasonerjt

@blackpiglet blackpiglet Awaiting requested review from blackpiglet

@Lyndon-Li Lyndon-Li Awaiting requested review from Lyndon-Li

Assignees

@qiuming-best qiuming-best

Labels
Documentation has-changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@qiuming-best @draghuram @sseago @kaovilai @shubham-pampattiwar @mateusoliveira43