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: allow injecting schema into lifecycle hook #4402

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

phisco
Copy link
Contributor

@phisco phisco commented Apr 29, 2024

An attempt at fixing the root cause of #4286, mostly a POC, I feel it could be improved.

Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
@github-actions github-actions bot added backport-requested ◀️ This pull request should be backported to all supported releases release-1.21 release-1.22 release-1.23 labels Apr 29, 2024
Copy link
Contributor

❗ By default, the pull request is configured to backport to all release branches.

  • To stop backporting this pr, remove the label: backport-requested ◀️ or add the label 'do not backport'
  • To stop backporting this pr to a certain release branch, remove the specific branch label: release-x.y

// Skip unknown object
return nil, nil
return object, nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is already covered by #4282, I do think it's the right thing to do.

@@ -144,7 +145,7 @@ func (data *data) LifecycleHook(
return object, nil
}

decoder := scheme.Codecs.UniversalDeserializer()
decoder := serializer.NewCodecFactory(scheme.Scheme).UniversalDeserializer()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

would be better to move this to a builder and only create it once.

ctx context.Context,
operationType plugin.OperationVerb,
schema *runtime.Scheme,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel it could be passed through a builder or something instead of adding one more parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-requested ◀️ This pull request should be backported to all supported releases release-1.21 release-1.22 release-1.23
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant