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

Draft: Enable WithIgnoreMutationWebhook #318

Closed
wants to merge 1 commit into from

Conversation

KalenWessel
Copy link
Collaborator

Add WithIgnoreMutationWebhook() to generateDiff function, this way we can have things like kyverno report back failed policy checks.

@KalenWessel KalenWessel force-pushed the support-mutationhooks branch 2 times, most recently from 876ec68 to 12e799d Compare December 4, 2024 20:10
… can have things like kyverno report back failed policy checks
@KalenWessel KalenWessel force-pushed the support-mutationhooks branch from 12e799d to ec511a9 Compare December 4, 2024 20:17
@KalenWessel KalenWessel changed the title Enable WithIgnoreMutationWebhook Draft: Enable WithIgnoreMutationWebhook Dec 4, 2024
@zapier-sre-bot
Copy link
Collaborator

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of pkg/checks/diff/diff.go

@@ -206,6 +206,7 @@ func generateDiff(ctx context.Context, request checks.Request, argoSettings *set
 		WithDiffSettings(request.App.Spec.IgnoreDifferences, overrides, ignoreAggregatedRoles, ignoreNormalizerOpts).
 		WithTracking(argoSettings.AppLabelKey, argoSettings.TrackingMethod).
 		WithNoCache().
+		WithIgnoreMutationWebhook(false).
 		Build()
 	if err != nil {
 		telemetry.SetError(span, err, "Build Diff")

Feedback & Suggestions:

  1. Security Consideration: The addition of WithIgnoreMutationWebhook(false) suggests that mutation webhooks will not be ignored during the diff process. Ensure that this change is intentional and that the application logic accounts for any side effects or security implications of processing mutation webhooks. Mutation webhooks can alter the state of resources, and ignoring them might have been a deliberate choice to avoid unintended changes.

  2. Documentation: Consider adding a comment explaining why WithIgnoreMutationWebhook(false) is being set. This will help future maintainers understand the rationale behind this decision and ensure that the change aligns with the intended behavior of the application.

  3. Testing: Ensure that there are adequate tests covering scenarios where mutation webhooks are involved. This will help verify that the new behavior does not introduce any regressions or unexpected behavior.



Dependency Review

Click to read mergecats review!

No suggestions found

@KalenWessel KalenWessel closed this Dec 4, 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