Skip to content

Commit c3e12c3

Browse files
pratap0007chmouel
pratap0007
authored and
chmouel
committed
fix: set the correct custom hub catalog type
Earlier, the custom hub catalog type was always set to artifacthub. With this fix, the custom hub catalog URL is validated and correct custom hub catalog type is set accordingly. Signed-off-by: Shiv Verma <[email protected]>
1 parent 3be8306 commit c3e12c3

File tree

7 files changed

+98
-28
lines changed

7 files changed

+98
-28
lines changed

‎pkg/cmd/tknpac/resolve/resolve.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ func Command(run *params.Run, streams *cli.IOStreams) *cobra.Command {
104104
return fmt.Errorf("you need to at least specify a file with -f")
105105
}
106106

107-
if err := settings.SyncConfig(run.Clients.Log, &run.Info.Pac.Settings, map[string]string{}, settings.DefaultValidators()); err != nil {
107+
if err := settings.SyncConfig(run.Clients.Log, &run.Info.Pac.Settings, map[string]string{}, settings.DefaultValidators(), &run.Clients.HTTP); err != nil {
108108
return err
109109
}
110110

‎pkg/params/info/info.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package info
22

33
import (
4+
"net/http"
45
"sync"
56

67
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings"
@@ -36,14 +37,14 @@ func (i *Info) GetPacOpts() PacOpts {
3637
return *i.Pac
3738
}
3839

39-
func (i *Info) UpdatePacOpts(logger *zap.SugaredLogger, configData map[string]string) (*settings.Settings, error) {
40+
func (i *Info) UpdatePacOpts(logger *zap.SugaredLogger, configData map[string]string, httpClient *http.Client) (*settings.Settings, error) {
4041
if i.pacMutex == nil {
4142
i.pacMutex = &sync.Mutex{}
4243
}
4344
i.pacMutex.Lock()
4445
defer i.pacMutex.Unlock()
4546

46-
if err := settings.SyncConfig(logger, &i.Pac.Settings, configData, settings.DefaultValidators()); err != nil {
47+
if err := settings.SyncConfig(logger, &i.Pac.Settings, configData, settings.DefaultValidators(), httpClient); err != nil {
4748
return nil, err
4849
}
4950
return &i.Pac.Settings, nil

‎pkg/params/run.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ func (r *Run) UpdatePacConfig(ctx context.Context) error {
2828
return err
2929
}
3030

31-
updatedPacInfo, err := r.Info.UpdatePacOpts(r.Clients.Log, cfg.Data)
31+
updatedPacInfo, err := r.Info.UpdatePacOpts(r.Clients.Log, cfg.Data, &r.Clients.HTTP)
3232
if err != nil {
3333
return err
3434
}

‎pkg/params/settings/config.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package settings
22

33
import (
44
"fmt"
5+
"net/http"
56
"net/url"
67
"regexp"
78
"strings"
@@ -112,8 +113,8 @@ func DefaultValidators() map[string]func(string) error {
112113
}
113114
}
114115

115-
func SyncConfig(logger *zap.SugaredLogger, setting *Settings, config map[string]string, validators map[string]func(string) error) error {
116-
setting.HubCatalogs = getHubCatalogs(logger, setting.HubCatalogs, config)
116+
func SyncConfig(logger *zap.SugaredLogger, setting *Settings, config map[string]string, validators map[string]func(string) error, httpClient *http.Client) error {
117+
setting.HubCatalogs = getHubCatalogs(logger, setting.HubCatalogs, config, httpClient)
117118

118119
err := configutil.ValidateAndAssignValues(logger, config, setting, validators, true)
119120
if err != nil {

‎pkg/params/settings/config_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package settings
22

33
import (
4+
"net/http"
45
"reflect"
56
"testing"
67

@@ -152,7 +153,7 @@ func TestSyncConfig(t *testing.T) {
152153
t.Run(tc.name, func(t *testing.T) {
153154
var test Settings
154155

155-
err := SyncConfig(logger, &test, tc.configMap, DefaultValidators())
156+
err := SyncConfig(logger, &test, tc.configMap, DefaultValidators(), http.DefaultClient)
156157

157158
// set hub catalogs to nil to avoid comparison error
158159
// test separately

‎pkg/params/settings/default.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import (
1313
"go.uber.org/zap"
1414
)
1515

16-
func getHubCatalogs(logger *zap.SugaredLogger, catalogs *sync.Map, config map[string]string) *sync.Map {
16+
func getHubCatalogs(logger *zap.SugaredLogger, catalogs *sync.Map, config map[string]string, httpClient *http.Client) *sync.Map {
1717
if catalogs == nil {
1818
catalogs = &sync.Map{}
1919
}
@@ -25,7 +25,7 @@ func getHubCatalogs(logger *zap.SugaredLogger, catalogs *sync.Map, config map[st
2525
if hubType, ok := config[HubCatalogTypeKey]; !ok || hubType == "" {
2626
config[HubCatalogTypeKey] = hubtypes.ArtifactHubType
2727
if config[HubURLKey] != "" {
28-
config[HubCatalogTypeKey] = getHubCatalogTypeViaAPI(config[HubURLKey])
28+
config[HubCatalogTypeKey] = getHubCatalogTypeViaAPI(config[HubURLKey], httpClient)
2929
}
3030
} else if hubType != hubtypes.ArtifactHubType && hubType != hubtypes.TektonHubType {
3131
logger.Warnf("CONFIG: invalid hub type %s, defaulting to %s", hubType, hubtypes.ArtifactHubType)
@@ -73,7 +73,7 @@ func getHubCatalogs(logger *zap.SugaredLogger, catalogs *sync.Map, config map[st
7373
catalogName := config[fmt.Sprintf("%s-name", cPrefix)]
7474
catalogType := config[fmt.Sprintf("%s-type", cPrefix)]
7575
if catalogType == "" {
76-
catalogType = hubtypes.ArtifactHubType // default to artifact hub if not specified
76+
catalogType = getHubCatalogTypeViaAPI(config[fmt.Sprintf("%s-url", cPrefix)], httpClient)
7777
}
7878

7979
value, ok := catalogs.Load(catalogID)
@@ -96,7 +96,7 @@ func getHubCatalogs(logger *zap.SugaredLogger, catalogs *sync.Map, config map[st
9696
return catalogs
9797
}
9898

99-
func getHubCatalogTypeViaAPI(hubURL string) string {
99+
func getHubCatalogTypeViaAPI(hubURL string, httpClient *http.Client) string {
100100
statsURL := fmt.Sprintf("%s/api/v1/stats", strings.TrimSuffix(hubURL, "/"))
101101

102102
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
@@ -107,7 +107,7 @@ func getHubCatalogTypeViaAPI(hubURL string) string {
107107
return hubtypes.TektonHubType
108108
}
109109

110-
resp, err := http.DefaultClient.Do(req)
110+
resp, err := httpClient.Do(req)
111111
if err != nil {
112112
return hubtypes.TektonHubType
113113
}

‎pkg/params/settings/default_test.go‎

Lines changed: 83 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,11 @@ package settings
22

33
import (
44
"net/http"
5-
"net/http/httptest"
65
"sync"
76
"testing"
87

98
hubtypes "github.com/openshift-pipelines/pipelines-as-code/pkg/hub/vars"
9+
testhttp "github.com/openshift-pipelines/pipelines-as-code/pkg/test/http"
1010
"go.uber.org/zap"
1111
zapobserver "go.uber.org/zap/zaptest/observer"
1212
"gotest.tools/v3/assert"
@@ -20,17 +20,37 @@ func TestGetCatalogHub(t *testing.T) {
2020
URL: "https://foo.com",
2121
Name: "tekton",
2222
})
23+
24+
// URLs for mocked HTTP responses
25+
const (
26+
artifactHubURL = "https://artifacthub.example.com"
27+
tektonHubURL = "https://tektonhub.example.com"
28+
)
29+
30+
// Mock HTTP client for API-based type detection
31+
mockHTTPClient := testhttp.MakeHTTPTestClient(map[string]map[string]string{
32+
artifactHubURL + "/api/v1/stats": {
33+
"code": "200",
34+
},
35+
tektonHubURL + "/api/v1/stats": {
36+
"code": "404",
37+
},
38+
})
39+
2340
tests := []struct {
24-
name string
25-
config map[string]string
26-
numCatalogs int
27-
wantLog string
28-
hubCatalogs *sync.Map
41+
name string
42+
config map[string]string
43+
numCatalogs int
44+
wantLog string
45+
hubCatalogs *sync.Map
46+
wantCustomType map[string]string
47+
httpClient *http.Client
2948
}{
3049
{
3150
name: "good/default catalog",
3251
numCatalogs: 1,
3352
hubCatalogs: &sync.Map{},
53+
httpClient: mockHTTPClient,
3454
},
3555
{
3656
name: "good/custom catalog",
@@ -42,6 +62,7 @@ func TestGetCatalogHub(t *testing.T) {
4262
numCatalogs: 2,
4363
hubCatalogs: &sync.Map{},
4464
wantLog: "CONFIG: setting custom hub custom, catalog https://foo.com",
65+
httpClient: mockHTTPClient,
4566
},
4667
{
4768
name: "good/custom catalog with same data",
@@ -53,6 +74,7 @@ func TestGetCatalogHub(t *testing.T) {
5374
numCatalogs: 2,
5475
hubCatalogs: &hubCatalog,
5576
wantLog: "",
77+
httpClient: mockHTTPClient,
5678
},
5779
{
5880
name: "good/custom catalog with different data",
@@ -64,6 +86,7 @@ func TestGetCatalogHub(t *testing.T) {
6486
numCatalogs: 2,
6587
hubCatalogs: &hubCatalog,
6688
wantLog: "CONFIG: setting custom hub custom, catalog https://bar.com",
89+
httpClient: mockHTTPClient,
6790
},
6891
{
6992
name: "good/custom catalog with initialization",
@@ -75,6 +98,7 @@ func TestGetCatalogHub(t *testing.T) {
7598
numCatalogs: 2,
7699
hubCatalogs: nil,
77100
wantLog: "CONFIG: setting custom hub custom, catalog https://foo.com",
101+
httpClient: mockHTTPClient,
78102
},
79103
{
80104
name: "bad/missing keys custom catalog",
@@ -85,6 +109,7 @@ func TestGetCatalogHub(t *testing.T) {
85109
numCatalogs: 1,
86110
hubCatalogs: &sync.Map{},
87111
wantLog: "CONFIG: hub 1 should have the key catalog-1-url, skipping catalog configuration",
112+
httpClient: mockHTTPClient,
88113
},
89114
{
90115
name: "bad/missing value for custom catalog",
@@ -96,6 +121,7 @@ func TestGetCatalogHub(t *testing.T) {
96121
numCatalogs: 1,
97122
hubCatalogs: &sync.Map{},
98123
wantLog: "CONFIG: hub 1 catalog configuration have empty value for key catalog-1-url, skipping catalog configuration",
124+
httpClient: mockHTTPClient,
99125
},
100126
{
101127
name: "bad/custom catalog called https",
@@ -107,6 +133,7 @@ func TestGetCatalogHub(t *testing.T) {
107133
numCatalogs: 1,
108134
hubCatalogs: &sync.Map{},
109135
wantLog: "CONFIG: custom hub catalog name cannot be https, skipping catalog configuration",
136+
httpClient: mockHTTPClient,
110137
},
111138
{
112139
name: "bad/invalid url",
@@ -118,6 +145,7 @@ func TestGetCatalogHub(t *testing.T) {
118145
numCatalogs: 1,
119146
hubCatalogs: &sync.Map{},
120147
wantLog: "catalog url /u1!@1!@#$afoo.com is not valid, skipping catalog configuration",
148+
httpClient: mockHTTPClient,
121149
},
122150
{
123151
name: "multiple catalogs with different types",
@@ -134,6 +162,7 @@ func TestGetCatalogHub(t *testing.T) {
134162
numCatalogs: 3, // default + 2 custom
135163
hubCatalogs: &sync.Map{},
136164
wantLog: "CONFIG: setting custom hub tektonhub, catalog https://tektonhub.com",
165+
httpClient: mockHTTPClient,
137166
},
138167
{
139168
name: "invalid hub type",
@@ -143,6 +172,35 @@ func TestGetCatalogHub(t *testing.T) {
143172
numCatalogs: 1,
144173
hubCatalogs: &sync.Map{},
145174
wantLog: `CONFIG: invalid hub type invalid, defaulting to artifacthub`,
175+
httpClient: mockHTTPClient,
176+
},
177+
{
178+
name: "custom catalog type detection via API - success (ArtifactHub)",
179+
config: map[string]string{
180+
"catalog-1-id": "example-ah",
181+
"catalog-1-url": artifactHubURL,
182+
"catalog-1-name": "artifact",
183+
},
184+
numCatalogs: 2,
185+
hubCatalogs: &sync.Map{},
186+
wantCustomType: map[string]string{
187+
"example-ah": hubtypes.ArtifactHubType,
188+
},
189+
httpClient: mockHTTPClient,
190+
},
191+
{
192+
name: "custom catalog type detection via API - failure (TektonHub)",
193+
config: map[string]string{
194+
"catalog-1-id": "example-th",
195+
"catalog-1-url": tektonHubURL,
196+
"catalog-1-name": "tekton",
197+
},
198+
numCatalogs: 2,
199+
hubCatalogs: &sync.Map{},
200+
wantCustomType: map[string]string{
201+
"example-th": hubtypes.TektonHubType,
202+
},
203+
httpClient: mockHTTPClient,
146204
},
147205
}
148206
for _, tt := range tests {
@@ -152,7 +210,7 @@ func TestGetCatalogHub(t *testing.T) {
152210
if tt.config == nil {
153211
tt.config = map[string]string{}
154212
}
155-
catalogs := getHubCatalogs(fakelogger, tt.hubCatalogs, tt.config)
213+
catalogs := getHubCatalogs(fakelogger, tt.hubCatalogs, tt.config, tt.httpClient)
156214
length := 0
157215
catalogs.Range(func(_, _ any) bool {
158216
length++
@@ -162,6 +220,13 @@ func TestGetCatalogHub(t *testing.T) {
162220
if tt.wantLog != "" {
163221
assert.Assert(t, len(catcher.FilterMessageSnippet(tt.wantLog).TakeAll()) > 0, "could not find log message: got ", catcher)
164222
}
223+
for catalogID, expectedType := range tt.wantCustomType {
224+
value, ok := catalogs.Load(catalogID)
225+
assert.Assert(t, ok, "catalog %s should exist", catalogID)
226+
catalog, ok := value.(HubCatalog)
227+
assert.Assert(t, ok, "catalog %s should be HubCatalog type", catalogID)
228+
assert.Equal(t, catalog.Type, expectedType)
229+
}
165230
cmp.Equal(catalogs, tt.hubCatalogs)
166231
})
167232
}
@@ -170,34 +235,36 @@ func TestGetCatalogHub(t *testing.T) {
170235
func TestGetHubCatalogTypeViaAPI(t *testing.T) {
171236
tests := []struct {
172237
name string
173-
serverStatus int
238+
serverStatus string
174239
expectedResult string
175240
}{
176241
{
177242
name: "returns ArtifactHubType on 200 OK",
178-
serverStatus: http.StatusOK,
243+
serverStatus: "200",
179244
expectedResult: hubtypes.ArtifactHubType,
180245
},
181246
{
182247
name: "returns TektonHubType on 404 Not Found",
183-
serverStatus: http.StatusNotFound,
248+
serverStatus: "404",
184249
expectedResult: hubtypes.TektonHubType,
185250
},
186251
{
187252
name: "returns TektonHubType on 500 Internal Server Error",
188-
serverStatus: http.StatusInternalServerError,
253+
serverStatus: "500",
189254
expectedResult: hubtypes.TektonHubType,
190255
},
191256
}
192257

193258
for _, tt := range tests {
194259
t.Run(tt.name, func(t *testing.T) {
195-
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
196-
w.WriteHeader(tt.serverStatus)
197-
}))
198-
defer server.Close()
260+
testURL := "https://test-hub.example.com"
261+
mockClient := testhttp.MakeHTTPTestClient(map[string]map[string]string{
262+
testURL + "/api/v1/stats": {
263+
"code": tt.serverStatus,
264+
},
265+
})
199266

200-
result := getHubCatalogTypeViaAPI(server.URL)
267+
result := getHubCatalogTypeViaAPI(testURL, mockClient)
201268
assert.Equal(t, result, tt.expectedResult)
202269
})
203270
}

0 commit comments

Comments
 (0)
⚡