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.
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
fix(#19314): allow ssh/altssh subdomains in repo URLs to match webhook payload #19315
fix(#19314): allow ssh/altssh subdomains in repo URLs to match webhook payload #19315
Changes from 6 commits
801aae4
3f8958f
c79b60d
858d6df
7746506
cf8ce08
1948699
7c6a3ab
eb25202
c4665d3
a58f88a
dba501e
40a16ce
5857b05
e3c9acb
f4e0773
30cc282
f9da773
27d62e2
7ba2875
d04bb80
0937a9a
b433ec0
3917438
717ecac
259ff98
0f36a7d
fb09082
7369ba2
10909de
3bd6d61
0520145
d70f91f
601d857
08e0a5f
6208575
1fc1257
ddade23
ac206d3
735fbc3
f359d34
12e827f
27ef474
046b24e
ae3e907
e9cee07
68309fc
6b55fe3
207dccf
ac143ad
d8438f0
c3396ab
cedbd1e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied directly from https://github.com/argoproj/argo-cd/blob/7746506/util/webhook/webhook.go#L41-L43.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we instead move this to a common util package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same thought but wasn’t sure if I should dig in that much. (Wasn’t sure if the 2 webhooks were being allowed to diverge intentionally for some reason.) There’s another new feature in the
Application
webhook (maxPayloadSizeMB
) that isn’t here either that I wanted to pull in.I’ll see what I can do about reusing as much as possible between the 2 webhooks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a similar PR open where the thing with the util package is already done (#16292). Maybe this can be used as an inspiration to these two PRs can be combined somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QuoteMeta
andSprintf
formatting copied from https://github.com/argoproj/argo-cd/blob/7746506/util/webhook/webhook.go#L348-L351.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto with this, if it's identical to the appset webhook handling logic, can we just move it to a shared location?