Add multi-URL support and per-resolution url param to Hub Resolver#9465
Add multi-URL support and per-resolution url param to Hub Resolver#9465ab-ghosh wants to merge 1 commit intotektoncd:mainfrom
Conversation
tekton-robot
added
the
release-note-none
tekton-robot
added
the
size/XXL
|
/kind feature |
tekton-robot
added
the
kind/feature
ab-ghosh
force-pushed
the
hub-resolver-multi-url
branch
from
8ba1f36 to
ea7a4f2
Compare
| // Precedence: ConfigMap YAML list > env var URL. | ||
| func resolveHubURLs(conf map[string]string, envVarURL, configKey string) []string { | ||
| if yamlStr, ok := conf[configKey]; ok && strings.TrimSpace(yamlStr) != "" { | ||
| urls, err := parseURLList(yamlStr) |
There was a problem hiding this comment.
if parseURLList fails here (bad YAML in the ConfigMap), the error gets swallowed and we silently fall back to the env var URL. Could be confusing for an operator who thinks they configured multi-URL but has a YAML typo. Could we at least log a warning so there's a breadcrumb?
There was a problem hiding this comment.
Yeah sure, will add a warning log here.
| return nil, fmt.Errorf("failed to parse URL list: %w", err) | ||
| } | ||
| for i, u := range urls { | ||
| urls[i] = strings.TrimRight(strings.TrimSpace(u), "/") |
There was a problem hiding this comment.
ConfigMap URLs from parseURLList don't go through any scheme validation. The url param gets checked for http/https in validateParams (resolver.go:368-376), but a ConfigMap entry like ftp://example.com would pass through unchecked. Worth adding the same validation here, and then documenting the constraint in hub-resolver.md alongside the ConfigMap options
There was a problem hiding this comment.
My understanding was that the ConfigMap URLs will usually be handled by the admins, while the url param is passed by the user, so I added the scheme validation there. Because of that I thought we could skip the validation for ConfigMap URLs. But yeah, it makes sense to keep the validation consistent there as well. I will add the same scheme validation for the ConfigMap URLs.
ab-ghosh
force-pushed
the
hub-resolver-multi-url
branch
from
ea7a4f2 to
bde40b1
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Implements per-resolution parameter
for explicit hub selection and ConfigMap-based URL lists with ordered
fallback for cluster-level defaults.
- Add param for per-resolution hub endpoint override
- Add / ConfigMap keys for
ordered URL lists with first-success-wins fallback
- Use TrimRight for trailing slash stripping on ConfigMap URLs
- Add unit tests for fallback, url override, error formatting
- Add e2e tests for url param and version constraint resolution
- Update docs with new param, ConfigMap options, and URL precedence
Signed-off-by: ab-ghosh <abghosh@redhat.com>
ab-ghosh
force-pushed
the
hub-resolver-multi-url
branch
from
bde40b1 to
af9fe92
Compare
3 participants
urlparameter to the Hub Resolver, allowing Pipeline authors to explicitly target a specific hub instance (Option A from Support multiple Artifact Hub URLs in the Hub Resolver #9353)artifact-hub-urls/tekton-hub-urlsConfigMap keys for configuring multiple hub URLs with ordered fallback — first successful responsewins (Option B from Support multiple Artifact Hub URLs in the Hub Resolver #9353)
urlparam > ConfigMap URL lists > env var > defaultChanges
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes