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

Update restore logic to allow Resource Modifiers to use data from the status field #8121

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

BaudouinH
Copy link

@BaudouinH BaudouinH commented Aug 16, 2024

Thank you for contributing to Velero!

Please add a summary of your change

This PR change slightly the restore logic to make it possible to use the status field in Restore Resource Modifiers.
The status field is now trimmed after applying the resource modifiers, not before, allowing patches in resource modifiers to reference the status field.

Does your change fix a particular issue?

Fixes #8094

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.

Signed-off-by: Baudouin Herlicq <[email protected]>
@BaudouinH BaudouinH force-pushed the allow-usage-of-status-field-in-resource-modifiers branch from b8e9fb0 to e90ddd4 Compare August 16, 2024 12:15
Signed-off-by: Baudouin Herlicq <[email protected]>
Signed-off-by: Baudouin Herlicq <[email protected]>
Copy link

codecov bot commented Aug 19, 2024

Codecov Report

Attention: Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 59.02%. Comparing base (07c03a8) to head (662049d).
Report is 10 commits behind head on main.

Files Patch % Lines
pkg/restore/restore.go 40.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8121      +/-   ##
==========================================
- Coverage   59.03%   59.02%   -0.02%     
==========================================
  Files         358      364       +6     
  Lines       30141    30272     +131     
==========================================
+ Hits        17795    17868      +73     
- Misses      10907    10958      +51     
- Partials     1439     1446       +7     

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

@@ -58,6 +58,11 @@ resourceModifierRules:
- copy
- test (covered below)

### Limitations of Resource Modifiers
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please move limitations section to the bottom of the docs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@BaudouinH, pl check this

}

ctx.log.Infof("restore status includes excludes: %+v", ctx.resourceStatusIncludesExcludes)

Copy link
Collaborator

Choose a reason for hiding this comment

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

the concern that @reasonerjt had in #8094 (comment) still holds.
Please pitch in others - CC: @sseago, if this could impact any RIA flows.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Otherwise please signoff - @reasonerjt

Copy link
Collaborator

Choose a reason for hiding this comment

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

@anshulahuja98 I'm unaware of any RIA that currently modifies status -- if I'm remembering correctly, don't we currently strip the status field before RIAs are run? I think that's one of the reasons we added unmodified item (itemFromBackup) to the RIA input, as it still has the full status available for plugins to reference.

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.

JSON patch of Restore resource modifiers failing when referencing fields from status
3 participants