-
Notifications
You must be signed in to change notification settings - Fork 264
feat: support self-hosted artifactory users #4945
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: fuskovic <[email protected]>
✅ Deploy Preview for docs-kargo-io ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4945 +/- ##
=======================================
Coverage 54.61% 54.62%
=======================================
Files 401 401
Lines 34813 34818 +5
=======================================
+ Hits 19014 19018 +4
Misses 14842 14842
- Partials 957 958 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: fuskovic <[email protected]>
Signed-off-by: fuskovic <[email protected]>
:::info | ||
The value of `X-Kargo-Repo-URLs` can either be a single repository URL | ||
or a comma-separated list of repository URLs. If set, any warehouses | ||
with subscriptions to the designated repository URL(s) will be | ||
refreshed. This can be useful for repositories with unconventional | ||
naming schemes or self-hosted instances. | ||
::: |
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.
A few things I would worry about:
If you register one webhook for many repos, you may have a lot of different repos to list here as part of the header value. Apart from being onerous, my worry is that it leads to unnecessary refresh of many Warehouses.
If wishing to avoid the problem above, it would compel a user to register many webhooks for one repo apiece, with just one repo URL in the header. This also seems onerous.
I'd initially been thinking we were only going to provide a way to relay the information that may be missing from the request's jpd_origin
field. i.e. A way to say, "here's the base domain name for this entire registry" and leave it at that.
But before we get to far into what the implementation options are here, I think we should wait for a response to this comment, because if an empty jdp_origin
field is the result of misconfiguration (or if not mis-configuration, still a problem that can be resolved via configuration) then we may not need a new header. We may be able to get by with simply adding a note to the docs about the need for jdp_origin
to not be empty and how to configure that correctly.
We may still need to do something with a new header, but let's table it until we have an answer to that comment.
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.
We had a user that had the repo key prepended as the subdomain. The one that was building a middleware service to intercept and modify the request before proxying to Kargo. The jdp_origin
field was set in his situation but that still doesn't solve his problem. Or other users in a similar situation for that matter. I think it's still a viable option for these kind of situations since they have no alternative.
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.
We spoke offline, but to summarize here for anyone following along, I agree that what you've PR'ed here solves for tricky cases that my original idea doesn't. So I'm not completely closed off to the idea. But I do want us to get more information about configuration (or mis-configuration, as the case may be) of self-hosted Artifactory and, ideally, get hands-on experience with it too before we commit to moving in any specific direction here.
Closes: #4899