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

[operator-opamp-bridge][chore]: Refactor OpAMPBridge Webhook #2288

Merged

Conversation

avadhut123pisal
Copy link
Contributor

Description:
This PR updates the OpAMPBridge Webhook code to make it consistent with the changes that are made in Collector webhook as a part of #2210.

Link to tracking Issue: NA

Testing: Tested locally.

Documentation: NA

@avadhut123pisal avadhut123pisal requested a review from a team October 28, 2023 08:55
@@ -137,7 +137,7 @@ webhooks:
namespace: system
path: /validate-opentelemetry-io-v1alpha1-opampbridge
failurePolicy: Fail
name: vopampbridge.kb.io
name: vopampbridgecreateupdate.kb.io
Copy link
Contributor

Choose a reason for hiding this comment

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

why did this name change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/avadhut123pisal/opentelemetry-operator/blob/c3088a72c7bd0af9f7d3283875cfdb860cc309f2/apis/v1alpha1/opampbridge_webhook.go#L37C1-L37C1

The change has been done considering the validation webhook configurations of Collector resource.
One webhook is for create & update operations (with failure policy as Fail) and another one is for delete operation (with failure policy as Ignore). Previously, by default there was a single webhook for all the operations.

Comment on lines +22 to +25
"github.com/go-logr/logr"

"github.com/open-telemetry/opentelemetry-operator/internal/config"

Copy link
Contributor

Choose a reason for hiding this comment

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

the structure of these imports is wrong, though idk why our linter isn't catching it...

Copy link
Contributor

Choose a reason for hiding this comment

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

im going to leave this in for now, and then we can figure out why the linter isn't catching this in a follow up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the structure of these imports is wrong, though idk why our linter isn't catching it...

Yeah. Sometimes, it throws error for correct import structure too.

@jaronoff97 jaronoff97 merged commit 65f0ba9 into open-telemetry:main Oct 31, 2023
26 checks passed
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants