Skip to content

Conversation

@dsuhinin
Copy link

@dsuhinin dsuhinin commented May 21, 2025

Merge ArgoCD and AppSet webhooks endpoints into single entry point.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

@bunnyshell
Copy link

bunnyshell bot commented May 21, 2025

❌ Preview Environment undeployed from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@dsuhinin dsuhinin changed the title Merge ArgoCD and AppSet webhooks endpoints into single entry point. feat: merge ArgoCD and AppSet webhooks endpoints into single entry point. May 21, 2025
Copy link
Member

@nitishfy nitishfy left a comment

Choose a reason for hiding this comment

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

I'd like to know a couple of things here:

  1. What's the motivation for this change? Is there an existing issue already? If no, please create an issue first before working on it.
  2. I see you've removed the debug logs from the code. Why is that necessary?

server/server.go Outdated
Comment on lines 1293 to 1310
mgr, err := ctrl.NewManager(cfg, ctrl.Options{
Scheme: scheme,
Metrics: metricsserver.Options{
BindAddress: server.ApplicationSetOpts.MetricsAddr,
},
Cache: cacheOpt,
HealthProbeBindAddress: server.ApplicationSetOpts.ProbeBindAddr,
LeaderElection: server.ApplicationSetOpts.EnableLeaderElection,
LeaderElectionID: "58ac56fa.applicationsets.argoproj.io",
Client: ctrlclient.Options{
DryRun: &server.ApplicationSetOpts.DryRun,
},
})
if err != nil {
log.Error(err, "unable to start manager")
os.Exit(1)
}
dynamicClient, err := dynamic.NewForConfig(mgr.GetConfig())
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the two-step of building a manager and then constructing a client from the manager's config can be avoided. That would allow us to eliminate a ton of config dependencies.

Copy link
Author

Choose a reason for hiding this comment

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

hm Im not sure but I can double check. I tried to use as minimum as possible but Ill double check it.

Copy link
Author

@dsuhinin dsuhinin May 29, 2025

Choose a reason for hiding this comment

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

@crenshaw-dev from what I can see we probably can drop. manager and client is using by next generators:

  • Clusters
  • ClusterDecisionResource
  • Plugin
    which shouldn't be a case when we are talking about webhook but I could be wrong.

@dsuhinin
Copy link
Author

dsuhinin commented May 29, 2025

I'd like to know a couple of things here:

  1. What's the motivation for this change? Is there an existing issue already? If no, please create an issue first before working on it.
  2. I see you've removed the debug logs from the code. Why is that necessary?

hey @nitishfy. sorry for the late response.

  1. yes, issues already exists -> Add AppSet webhook endpoint in argocd-server #13764 and Provide a single entrypoint for all public webhooks #14954. also we spoke with @crenshaw-dev directly and it seems like it is reasonable to try to merge argocd and appset webhooks into one single entry point and stay old one appset webhook for back compatibility.
  2. I've removed some logging, because now Handler functions started returning an error, which will be logged here:
for name, h := range map[string]func(w http.ResponseWriter, r *http.Request) error{
 		"argo cd":         m.acdWebhookHandler.HandleRequest,
 		"application set": m.appSetWebhookHandler.HandleRequest,
 	} {
 		go func() {
 			req, err := copyRequest(r)
 			if err == nil {
 				if err = h(w, req); err != nil {
 					log.Error("error handling %s webhook: %+v. maybe not suitable?", name, err)
 				}
 			} else {
 				log.Error("error copying request: %+v", err)
 			}
 		}()
 	}

@crenshaw-dev crenshaw-dev added this to the v3.1 milestone Jun 3, 2025
@codecov
Copy link

codecov bot commented Jun 10, 2025

Codecov Report

Attention: Patch coverage is 84.11215% with 17 lines in your changes missing coverage. Please review.

Project coverage is 60.10%. Comparing base (6b24fcb) to head (a9d7829).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
applicationset/webhook/webhook.go 76.19% 4 Missing and 1 partial ⚠️
cmd/argocd-server/commands/argocd_server.go 0.00% 4 Missing ⚠️
server/server.go 90.00% 3 Missing and 1 partial ⚠️
util/webhookmerger/webhookmerger.go 83.33% 3 Missing ⚠️
util/webhook/webhook.go 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #23081      +/-   ##
==========================================
+ Coverage   60.05%   60.10%   +0.05%     
==========================================
  Files         342      344       +2     
  Lines       58820    58909      +89     
==========================================
+ Hits        35325    35409      +84     
- Misses      20635    20642       +7     
+ Partials     2860     2858       -2     

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

@agaudreault agaudreault modified the milestones: v3.1, v3.2 Jun 13, 2025
@dsuhinin dsuhinin force-pushed the provide-single-entrypoint-for-all-public-webhooks branch from 4ef7153 to 8780da6 Compare June 16, 2025 06:40
gekart and others added 20 commits June 16, 2025 14:26
Signed-off-by: Michael Crenshaw <[email protected]>
Signed-off-by: Grischa Ekart <[email protected]>
Signed-off-by: Dan Garfield <[email protected]>
Co-authored-by: Michael Crenshaw <[email protected]>
Co-authored-by: Dan Garfield <[email protected]>
Signed-off-by: dsuhinin <[email protected]>
Signed-off-by: Michael Crenshaw <[email protected]>
Signed-off-by: dsuhinin <[email protected]>
Signed-off-by: Michael Crenshaw <[email protected]>
Signed-off-by: dsuhinin <[email protected]>
…j#22965)

Signed-off-by: oleksandr-codefresh <[email protected]>
Co-authored-by: Regina Voloshin <[email protected]>
Signed-off-by: dsuhinin <[email protected]>
…/remote (argoproj#22985)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dsuhinin <[email protected]>
…st (argoproj#22986)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dsuhinin <[email protected]>
…rgoproj#22987)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dsuhinin <[email protected]>
…roj#22988)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dsuhinin <[email protected]>
…goproj#22989)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dsuhinin <[email protected]>
Signed-off-by: Karsten Thoms <[email protected]>
Co-authored-by: Dan Garfield <[email protected]>
Signed-off-by: dsuhinin <[email protected]>
Signed-off-by: Boxuan Tang <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Co-authored-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: dsuhinin <[email protected]>
…om 1.9.0 to 1.10.0 (argoproj#22990)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dsuhinin <[email protected]>
…oproj#22813)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dsuhinin <[email protected]>
crenshaw-dev and others added 19 commits June 16, 2025 14:33
 argoproj#20785, argoproj#18478) (argoproj#22713)

Signed-off-by: Hazel Sudzilouski <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Co-authored-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: dsuhinin <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: dsuhinin <[email protected]>
Signed-off-by: Christian Hernandez <[email protected]>
Co-authored-by: Blake Pettersson <[email protected]>
Signed-off-by: dsuhinin <[email protected]>
Signed-off-by: Blake Pettersson <[email protected]>
Signed-off-by: dsuhinin <[email protected]>
Signed-off-by: Michael Crenshaw <[email protected]>
Signed-off-by: dsuhinin <[email protected]>
…lingSync (argoproj#20428) (argoproj#23335)

Signed-off-by: Mike Ng <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Co-authored-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: dsuhinin <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: dsuhinin <[email protected]>
Signed-off-by: CI <[email protected]>
Co-authored-by: CI <[email protected]>
Signed-off-by: dsuhinin <[email protected]>
@dsuhinin dsuhinin force-pushed the provide-single-entrypoint-for-all-public-webhooks branch from 67d8546 to 70e2825 Compare June 16, 2025 12:34
@dsuhinin dsuhinin marked this pull request as ready for review June 16, 2025 15:16
@dsuhinin dsuhinin requested review from a team as code owners June 16, 2025 15:16
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.