[chore] Refactor Webhooks to their own packages #2210
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jaronoff97
commented
Oct 11, 2023
config/webhook/manifests.yaml
Outdated
There was a problem hiding this comment.
The order of these is reversing, but is a noop AFAICT
jaronoff97
commented
Oct 11, 2023
| if inst.Spec.Nginx.Image == autoInstNginx { | ||
| inst.Spec.Nginx.Image = u.DefaultAutoInstNginx | ||
| inst.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationNginx] = u.DefaultAutoInstNginx | ||
| func (u *InstrumentationUpgrade) upgrade(_ context.Context, inst v1alpha1.Instrumentation) *v1alpha1.Instrumentation { |
There was a problem hiding this comment.
the previous way was really annoying me. If anyone has problems with this, i am happy to revert it.
pavolloffay
reviewed
Oct 12, 2023
config/webhook/manifests.yaml
Outdated
| @@ -10,9 +10,9 @@ webhooks: | |||
| service: | |||
| name: webhook-service | |||
| namespace: system | |||
| path: /mutate-opentelemetry-io-v1alpha1-instrumentation | |||
| path: /mutate-opentelemetry-io-v1alpha1-opentelemetrycollector | |||
There was a problem hiding this comment.
why are these changes here?
I am concerned that this will cause issues for the upgrade (also the upgrade test was changed )
…or into refactor-webhooks-pt1
…or into refactor-webhooks-pt1
…or into refactor-webhooks-pt1
|
@pavolloffay should be good to go now! |
pavolloffay
requested changes
Oct 16, 2023
| // Package webhookhandler contains the webhook that injects sidecars into pods. | ||
| package webhookhandler | ||
| // Package sidecar contains the webhook that injects sidecars into pods. | ||
| package sidecar |
There was a problem hiding this comment.
I don't think we should call this package sidecar
The package exposes a mutator API that allows mutating pod objects. It is used for instrumentation and sidecar
pavolloffay
approved these changes
Oct 17, 2023
ItielOlenick
pushed a commit
to ItielOlenick/opentelemetry-operator
that referenced
this pull request
May 1, 2024
* lots of moving things around and simplifcation * moves webhook manifest * rename * generate * Add back annotations * Fix tests, simplify code * naming * fix borked test * FIX TESTS * oops * move things back * update manifests * fix a miss * fix tests * Rename
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
This PR updates how webhooks are created and managed by the operator by moving them out of types and into an internal package. In the future this will let us add more complex logic to the webhooks, but for now this mostly is a no-op. One benefit for instrumentation webhooks is that we no longer need to add the annotations to the resource for defaulting because we can add a config field to the instrumentation webhook. All tests have been moved over and replaced with the actual webhooks.
Link to tracking Issue: n/a
Testing: tested locally.
Documentation: n/a