From 789eeef8891b0ef4da5874c262901b2efdc42b51 Mon Sep 17 00:00:00 2001 From: rzarkar Date: Thu, 15 Oct 2026 15:02:24 -0400 Subject: [PATCH 1/4] expose remote config loading --- README.md | 16 +++ appconfig/appconfig.go | 24 +++- appconfig/options.go | 3 +- appconfig/remote_test.go | 245 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 286 insertions(+), 2 deletions(-) create mode 100644 appconfig/remote_test.go diff --git a/README.md b/README.md index ba12466e..3d4f2ef4 100644 --- a/README.md +++ b/README.md @@ -332,6 +332,22 @@ func loadConfig(ctx context.Context, client *github.Client, owner, repo, ref str } ``` +To load a specific remote file without discovery or remote-reference parsing, use +`LoadRemoteRef`. Missing files are errors; existing empty files retain their +source metadata. + +```go +c, err := loader.LoadRemoteRef(ctx, client, clientOwner, appconfig.RemoteRef{ + Remote: "example/shared-config", + Path: "config/app.yml", + Ref: "main", +}) +``` + +`clientOwner` must be the supplied client's original installation owner, even if +an earlier load returned a file owned by another organization. Cross-owner +installation access requires `WithPrivateRemotes` + ## OAuth2 The `oauth2` package provides an `http.Handler` implementation that simplifies diff --git a/appconfig/appconfig.go b/appconfig/appconfig.go index b10238cf..987be8f7 100644 --- a/appconfig/appconfig.go +++ b/appconfig/appconfig.go @@ -24,6 +24,7 @@ import ( "fmt" "io" "net/http" + "net/url" "strings" "github.com/google/go-github/v92/github" @@ -55,7 +56,14 @@ func (r RemoteRef) SplitRemote() (owner, repo string, err error) { if slash <= 0 || slash >= len(r.Remote)-1 { return "", "", fmt.Errorf("invalid remote value: %s", r.Remote) } - return r.Remote[:slash], r.Remote[slash+1:], nil + owner, repo = r.Remote[:slash], r.Remote[slash+1:] + // github clients turn these into urls, so ensuring valid URLs + for _, part := range []string{owner, repo} { + if part == "." || part == ".." || url.PathEscape(part) != part { + return "", "", fmt.Errorf("invalid remote value: %s", r.Remote) + } + } + return owner, repo, nil } // Config contains unparsed configuration data and metadata about where it was found. @@ -163,6 +171,20 @@ func (ld *Loader) LoadConfig(ctx context.Context, client *github.Client, owner, return Config{}, nil } +// LoadRemoteRef returns a literal file without parsing references or searching +// other paths or owner defaults. Path and ref defaults are defined by [RemoteRef]. +// Missing files are errors; empty files retain their source metadata. +// +// clientOwner must be client's original installation owner, not a loaded file's +// owner. It is trusted caller context. See [WithPrivateRemotes] for cross-owner +// access and its information disclosure warning. +// +// On load errors, Config identifies the remote file, with a ref once resolved. +// An invalid remote repository value returns an empty Config and an error. +func (ld *Loader) LoadRemoteRef(ctx context.Context, client *github.Client, clientOwner string, remote RemoteRef) (Config, error) { + return ld.loadRemoteConfig(ctx, client, clientOwner, remote, Config{}) +} + func (ld *Loader) loadRemoteConfig(ctx context.Context, client *github.Client, sourceOwner string, remote RemoteRef, c Config) (Config, error) { logger := zerolog.Ctx(ctx) notFoundErr := fmt.Errorf("invalid remote reference: file does not exist") diff --git a/appconfig/options.go b/appconfig/options.go index 7285d443..508d43a0 100644 --- a/appconfig/options.go +++ b/appconfig/options.go @@ -21,7 +21,8 @@ import ( type Option func(*Loader) // WithRemoteRefParser sets the parser for encoded RemoteRefs. The default -// parser uses YAML. Set a nil parser to disable remote references. +// parser uses YAML. Set a nil parser to disable automatic remote references in +// LoadConfig. func WithRemoteRefParser(parser RemoteRefParser) Option { return func(ld *Loader) { ld.parser = parser diff --git a/appconfig/remote_test.go b/appconfig/remote_test.go new file mode 100644 index 00000000..775d9dbb --- /dev/null +++ b/appconfig/remote_test.go @@ -0,0 +1,245 @@ +// Copyright 2026 Palantir Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package appconfig + +import ( + "context" + "encoding/base64" + "errors" + "net/http" + "reflect" + "testing" + + "github.com/google/go-github/v92/github" + "github.com/palantir/go-githubapp/githubapp" +) + +type remoteRoundTripperFunc func(*http.Request) (*http.Response, error) + +func (f remoteRoundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { + return f(req) +} + +func newRemoteTestClient(t *testing.T, rt http.RoundTripper) *github.Client { + t.Helper() + client, err := github.NewClient(github.WithHTTPClient(&http.Client{Transport: rt})) + if err != nil { + t.Fatal(err) + } + return client +} + +func TestLoadRemoteRefLiteral(t *testing.T) { + content := "remote: remote/config\npath: first.yml\nref: main\n" + var requests []string + client := newRemoteTestClient(t, remoteRoundTripperFunc(func(req *http.Request) (*http.Response, error) { + requests = append(requests, req.URL.RequestURI()) + response := SavedResponse{Status: http.StatusOK, Body: `{"default_branch":"main"}`} + if req.URL.Path != "/repos/remote/config" { + response.Body = `{"encoding":"base64","content":"` + base64.StdEncoding.EncodeToString([]byte(content)) + `"}` + } + return response.Response(req), nil + })) + loader := NewLoader([]string{"first.yml", "second.yml"}, WithRemoteRefParser(func(string, []byte) (*RemoteRef, error) { + t.Error("literal loading invoked the remote parser") + return nil, errors.New("unexpected parser call") + })) + + cfg, err := loader.LoadRemoteRef(context.Background(), client, "original", RemoteRef{Remote: "remote/config"}) + if err != nil { + t.Fatal(err) + } + want := Config{Content: []byte(content), Source: "remote/config@main", Path: "first.yml", IsRemote: true} + if !reflect.DeepEqual(cfg, want) { + t.Errorf("got %+v, want %+v", cfg, want) + } + wantRequests := []string{"/repos/remote/config", "/repos/remote/config/contents/first.yml?ref=main"} + if !reflect.DeepEqual(requests, wantRequests) { + t.Errorf("requests: got %v, want %v", requests, wantRequests) + } +} + +func TestLoadRemoteRefMissingAndEmpty(t *testing.T) { + for name, test := range map[string]struct { + response SavedResponse + missing bool + }{ + "missing target": {response: SavedResponse{Status: http.StatusNotFound}, missing: true}, + "directory": {response: SavedResponse{Status: http.StatusOK, Body: `[]`}, missing: true}, + "empty file": {response: SavedResponse{Status: http.StatusOK, Body: `{"encoding":"base64","content":""}`}}, + } { + t.Run(name, func(t *testing.T) { + calls := 0 + client := newRemoteTestClient(t, remoteRoundTripperFunc(func(req *http.Request) (*http.Response, error) { + calls++ + if req.URL.RequestURI() != "/repos/remote/config/contents/first.yml?ref=main" { + t.Errorf("unexpected request: %s", req.URL) + } + return test.response.Response(req), nil + })) + loader := NewLoader([]string{"first.yml", "second.yml"}) + cfg, err := loader.LoadRemoteRef(context.Background(), client, "original", RemoteRef{Remote: "remote/config", Ref: "main"}) + if test.missing { + if err == nil || err.Error() != "invalid remote reference: file does not exist" { + t.Fatalf("expected missing remote error, got %v", err) + } + } else if err != nil { + t.Fatal(err) + } + if calls != 1 || cfg.IsUndefined() || len(cfg.Content) != 0 || cfg.Source != "remote/config@main" || cfg.Path != "first.yml" || !cfg.IsRemote { + t.Errorf("unexpected config or request count: %+v, %d", cfg, calls) + } + }) + } +} + +func TestLoadRemoteRefInvalidRepository(t *testing.T) { + for _, remote := range []string{ + "", "owner", "/repo", "owner/", "owner/repo/extra", + "owner/repo#", "owner/repo?x=", "owner/repo/contents/other.yml#", + "owner#fragment/repo", "owner%2frepo/name", "owner/repo%23fragment", + "./repo", "../repo", "owner/.", "owner/..", "owner/%2e%2e", + "https://example.invalid/repo", "owner/repo\\extra", "owner/repo name", + } { + t.Run(remote, func(t *testing.T) { + client := newRemoteTestClient(t, remoteRoundTripperFunc(func(req *http.Request) (*http.Response, error) { + t.Error("invalid remote made an HTTP request") + // Query/fragment injection previously made this look like an empty file. + return (&SavedResponse{Status: http.StatusOK, Body: `{"name":"repo"}`}).Response(req), nil + })) + installations := &testInstallationsService{} + creator := &testClientCreator{client: client} + loader := NewLoader(nil, WithPrivateRemotes(creator, installations)) + cfg, err := loader.LoadRemoteRef(context.Background(), client, "original", RemoteRef{Remote: remote, Path: "app.yml", Ref: "main"}) + if err == nil || !reflect.DeepEqual(cfg, Config{}) { + t.Errorf("expected invalid remote error and empty Config, got %+v, %v", cfg, err) + } + if installations.calls != 0 || creator.calls != 0 { + t.Errorf("invalid remote triggered installation lookup/client creation: %d, %d", installations.calls, creator.calls) + } + }) + } +} + +func TestLoadRemoteRefSuccessivePrivateFiles(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + original := newRemoteTestClient(t, remoteRoundTripperFunc(func(req *http.Request) (*http.Response, error) { + t.Error("used original client for private remote") + return errorResponse(req, http.StatusForbidden, "denied") + })) + var requests []string + selected := newRemoteTestClient(t, remoteRoundTripperFunc(func(req *http.Request) (*http.Response, error) { + requests = append(requests, req.URL.String()) + if req.Context() != ctx { + t.Error("selected client lost caller context") + } + response := SavedResponse{Status: http.StatusOK} + switch req.URL.Path { + case "/repos/remote/config/contents/child.yml": + response.Body = `{"encoding":"base64","content":"Y2hpbGQ="}` + case "/repos/remote/config/contents/parent.yml": + response.Body = `{"encoding":"none","download_url":"https://download.example/parent"}` + case "/parent": + response.Body = "parent" + } + return response.Response(req), nil + })) + installations := &testInstallationsService{installation: githubapp.Installation{ID: 42}} + creator := &testClientCreator{client: selected} + loader := NewLoader([]string{"unused.yml"}, WithPrivateRemotes(creator, installations)) + + for _, file := range []string{"child", "parent"} { + cfg, err := loader.LoadRemoteRef(ctx, original, "source", RemoteRef{Remote: "remote/config", Path: file + ".yml", Ref: "main"}) + if err != nil || string(cfg.Content) != file || cfg.Source != "remote/config@main" || cfg.Path != file+".yml" || !cfg.IsRemote { + t.Fatalf("incorrect remote config: %+v, %v", cfg, err) + } + } + if installations.calls != 2 || installations.owner != "remote" || installations.repo != "config" || creator.calls != 2 || creator.installationID != 42 { + t.Errorf("incorrect installation selection: lookup=%+v creator=%+v", installations, creator) + } + wantRequests := []string{ + "https://api.github.com/repos/remote/config/contents/child.yml?ref=main", + "https://api.github.com/repos/remote/config/contents/parent.yml?ref=main", + "https://download.example/parent", + } + if !reflect.DeepEqual(requests, wantRequests) { + t.Errorf("requests: got %v, want %v", requests, wantRequests) + } +} + +func TestLoadRemoteRefAccessFailures(t *testing.T) { + for name, test := range map[string]struct { + lookupErr, creatorErr error + originalStatus int + originalCalls, clientCalls int + creations int + }{ + "lookup failure": {lookupErr: githubapp.InstallationNotFound("remote/config"), originalStatus: http.StatusForbidden, originalCalls: 1}, + "creation failure": {creatorErr: errors.New("creation failed"), originalStatus: http.StatusForbidden, originalCalls: 1, creations: 1}, + "read failure": {originalStatus: http.StatusOK, clientCalls: 1, creations: 1}, + } { + t.Run(name, func(t *testing.T) { + originalCalls, selectedCalls := 0, 0 + original := newRemoteTestClient(t, remoteRoundTripperFunc(func(req *http.Request) (*http.Response, error) { + originalCalls++ + return (&SavedResponse{Status: test.originalStatus, Body: `{"encoding":"base64","content":""}`}).Response(req), nil + })) + selected := newRemoteTestClient(t, remoteRoundTripperFunc(func(req *http.Request) (*http.Response, error) { + selectedCalls++ + return errorResponse(req, http.StatusForbidden, "denied") + })) + installations := &testInstallationsService{installation: githubapp.Installation{ID: 42}, err: test.lookupErr} + creator := &testClientCreator{client: selected, err: test.creatorErr} + loader := NewLoader(nil, WithPrivateRemotes(creator, installations)) + _, err := loader.LoadRemoteRef(context.Background(), original, "source", RemoteRef{Remote: "remote/config", Path: "app.yml", Ref: "main"}) + var responseErr *github.ErrorResponse + if !errors.As(err, &responseErr) || responseErr.Response.StatusCode != http.StatusForbidden { + t.Fatalf("expected access denial, got %v", err) + } + if originalCalls != test.originalCalls || selectedCalls != test.clientCalls || installations.calls != 1 || creator.calls != test.creations { + t.Errorf("incorrect access attempts: original=%d selected=%d lookup=%d creation=%d", originalCalls, selectedCalls, installations.calls, creator.calls) + } + }) + } +} + +func TestLoadConfigInvalidRemoteDiagnostics(t *testing.T) { + content := "remote: remote/config#\n" + client := newRemoteTestClient(t, remoteRoundTripperFunc(func(req *http.Request) (*http.Response, error) { + response := SavedResponse{Status: http.StatusOK} + if req.URL.Path == "/repos/original/.github" { + response.Body = `{"name":".github","default_branch":"main"}` + } else { + response.Body = `{"encoding":"base64","content":"` + base64.StdEncoding.EncodeToString([]byte(content)) + `"}` + } + return response.Response(req), nil + })) + for _, test := range []struct { + name, source string + paths []string + }{ + {name: "local", source: "original/local@main", paths: []string{"app.yml"}}, + {name: "owner default", source: "original/.github@main"}, + } { + t.Run(test.name, func(t *testing.T) { + loader := NewLoader(test.paths, WithOwnerDefault(".github", []string{"app.yml"})) + cfg, err := loader.LoadConfig(context.Background(), client, "original", "local", "main") + if err == nil || err.Error() != "invalid remote value: remote/config#" || cfg.Source != test.source || cfg.Path != "app.yml" || cfg.IsRemote { + t.Fatalf("lost originating file diagnostics: %+v, %v", cfg, err) + } + }) + } +} From 295a8671750654e59f16933e686a444fdff0997a Mon Sep 17 00:00:00 2001 From: rzarkar Date: Wed, 28 Oct 2026 22:12:10 -0400 Subject: [PATCH 2/4] Clarify remote loader installation documentation --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 3d4f2ef4..c0844132 100644 --- a/README.md +++ b/README.md @@ -345,8 +345,8 @@ c, err := loader.LoadRemoteRef(ctx, client, clientOwner, appconfig.RemoteRef{ ``` `clientOwner` must be the supplied client's original installation owner, even if -an earlier load returned a file owned by another organization. Cross-owner -installation access requires `WithPrivateRemotes` +an earlier load returned a file owned by another organization. Using another +owner's installation requires `appconfig.WithPrivateRemotes`. ## OAuth2 From 43cde26b8ba5a498508509d0fc1e34eaf4d038d2 Mon Sep 17 00:00:00 2001 From: rzarkar Date: Thu, 5 Nov 2026 13:00:42 -0500 Subject: [PATCH 3/4] keep remote loading focused --- appconfig/appconfig.go | 24 +++------ appconfig/remote_test.go | 110 ++++++++++++++++++++++----------------- 2 files changed, 69 insertions(+), 65 deletions(-) diff --git a/appconfig/appconfig.go b/appconfig/appconfig.go index b3078271..9947ab98 100644 --- a/appconfig/appconfig.go +++ b/appconfig/appconfig.go @@ -24,7 +24,6 @@ import ( "fmt" "io" "net/http" - "net/url" "strings" "github.com/google/go-github/v93/github" @@ -56,14 +55,7 @@ func (r RemoteRef) SplitRemote() (owner, repo string, err error) { if slash <= 0 || slash >= len(r.Remote)-1 { return "", "", fmt.Errorf("invalid remote value: %s", r.Remote) } - owner, repo = r.Remote[:slash], r.Remote[slash+1:] - // github clients turn these into urls, so ensuring valid URLs - for _, part := range []string{owner, repo} { - if part == "." || part == ".." || url.PathEscape(part) != part { - return "", "", fmt.Errorf("invalid remote value: %s", r.Remote) - } - } - return owner, repo, nil + return r.Remote[:slash], r.Remote[slash+1:], nil } // Config contains unparsed configuration data and metadata about where it was found. @@ -171,16 +163,12 @@ func (ld *Loader) LoadConfig(ctx context.Context, client *github.Client, owner, return Config{}, nil } -// LoadRemoteRef returns a literal file without parsing references or searching -// other paths or owner defaults. Path and ref defaults are defined by [RemoteRef]. -// Missing files are errors; empty files retain their source metadata. -// -// clientOwner must be client's original installation owner, not a loaded file's -// owner. It is trusted caller context. See [WithPrivateRemotes] for cross-owner -// access and its information disclosure warning. +// LoadRemoteRef loads the file identified by remote. Unlike LoadConfig, it +// does not follow remote references, try other paths, or fall back to owner +// defaults. A missing file is an error. // -// On load errors, Config identifies the remote file, with a ref once resolved. -// An invalid remote repository value returns an empty Config and an error. +// clientOwner is the owner of the installation for that client. +// It determines whether WithPrivateRemotes selects a different installation. func (ld *Loader) LoadRemoteRef(ctx context.Context, client *github.Client, clientOwner string, remote RemoteRef) (Config, error) { return ld.loadRemoteConfig(ctx, client, clientOwner, remote, Config{}) } diff --git a/appconfig/remote_test.go b/appconfig/remote_test.go index 2b79b747..4c2417a7 100644 --- a/appconfig/remote_test.go +++ b/appconfig/remote_test.go @@ -98,36 +98,50 @@ func TestLoadRemoteRefMissingAndEmpty(t *testing.T) { } else if err != nil { t.Fatal(err) } - if calls != 1 || cfg.IsUndefined() || len(cfg.Content) != 0 || cfg.Source != "remote/config@main" || cfg.Path != "first.yml" || !cfg.IsRemote { - t.Errorf("unexpected config or request count: %+v, %d", cfg, calls) + if calls != 1 { + t.Errorf("expected one request, got %d", calls) + } + if cfg.IsUndefined() { + t.Error("expected config to retain source metadata") + } + if len(cfg.Content) != 0 { + t.Errorf("expected empty content, got %q", cfg.Content) + } + if cfg.Source != "remote/config@main" { + t.Errorf("incorrect source: %q", cfg.Source) + } + if cfg.Path != "first.yml" { + t.Errorf("incorrect path: %q", cfg.Path) + } + if !cfg.IsRemote { + t.Error("expected remote config") } }) } } func TestLoadRemoteRefInvalidRepository(t *testing.T) { - for _, remote := range []string{ - "", "owner", "/repo", "owner/", "owner/repo/extra", - "owner/repo#", "owner/repo?x=", "owner/repo/contents/other.yml#", - "owner#fragment/repo", "owner%2frepo/name", "owner/repo%23fragment", - "./repo", "../repo", "owner/.", "owner/..", "owner/%2e%2e", - "https://example.invalid/repo", "owner/repo\\extra", "owner/repo name", - } { + for _, remote := range []string{"", "owner", "/repo", "owner/"} { t.Run(remote, func(t *testing.T) { client := newRemoteTestClient(t, remoteRoundTripperFunc(func(req *http.Request) (*http.Response, error) { t.Error("invalid remote made an HTTP request") - // Query/fragment injection previously made this look like an empty file. return (&SavedResponse{Status: http.StatusOK, Body: `{"name":"repo"}`}).Response(req), nil })) installations := &testInstallationsService{} creator := &testClientCreator{client: client} loader := NewLoader(nil, WithPrivateRemotes(creator, installations)) cfg, err := loader.LoadRemoteRef(context.Background(), client, "original", RemoteRef{Remote: remote, Path: "app.yml", Ref: "main"}) - if err == nil || !reflect.DeepEqual(cfg, Config{}) { - t.Errorf("expected invalid remote error and empty Config, got %+v, %v", cfg, err) + if err == nil { + t.Error("expected invalid remote error") + } + if !reflect.DeepEqual(cfg, Config{}) { + t.Errorf("expected empty Config, got %+v", cfg) + } + if installations.calls != 0 { + t.Errorf("invalid remote triggered %d installation lookups", installations.calls) } - if installations.calls != 0 || creator.calls != 0 { - t.Errorf("invalid remote triggered installation lookup/client creation: %d, %d", installations.calls, creator.calls) + if creator.calls != 0 { + t.Errorf("invalid remote triggered %d client creations", creator.calls) } }) } @@ -163,12 +177,33 @@ func TestLoadRemoteRefSuccessivePrivateFiles(t *testing.T) { for _, file := range []string{"child", "parent"} { cfg, err := loader.LoadRemoteRef(ctx, original, "source", RemoteRef{Remote: "remote/config", Path: file + ".yml", Ref: "main"}) - if err != nil || string(cfg.Content) != file || cfg.Source != "remote/config@main" || cfg.Path != file+".yml" || !cfg.IsRemote { - t.Fatalf("incorrect remote config: %+v, %v", cfg, err) + if err != nil { + t.Fatal(err) } + if string(cfg.Content) != file { + t.Errorf("incorrect content: %q", cfg.Content) + } + if cfg.Source != "remote/config@main" { + t.Errorf("incorrect source: %q", cfg.Source) + } + if cfg.Path != file+".yml" { + t.Errorf("incorrect path: %q", cfg.Path) + } + if !cfg.IsRemote { + t.Error("expected remote config") + } + } + if installations.calls != 2 { + t.Errorf("expected two installation lookups, got %d", installations.calls) + } + if installations.owner != "remote" || installations.repo != "config" { + t.Errorf("incorrect installation repository: %s/%s", installations.owner, installations.repo) + } + if creator.calls != 2 { + t.Errorf("expected two client creations, got %d", creator.calls) } - if installations.calls != 2 || installations.owner != "remote" || installations.repo != "config" || creator.calls != 2 || creator.installationID != 42 { - t.Errorf("incorrect installation selection: lookup=%+v creator=%+v", installations, creator) + if creator.installationID != 42 { + t.Errorf("incorrect installation ID: %d", creator.installationID) } wantRequests := []string{ "https://api.github.com/repos/remote/config/contents/child.yml?ref=main", @@ -209,36 +244,17 @@ func TestLoadRemoteRefAccessFailures(t *testing.T) { if !errors.As(err, &responseErr) || responseErr.Response.StatusCode != http.StatusForbidden { t.Fatalf("expected access denial, got %v", err) } - if originalCalls != test.originalCalls || selectedCalls != test.clientCalls || installations.calls != 1 || creator.calls != test.creations { - t.Errorf("incorrect access attempts: original=%d selected=%d lookup=%d creation=%d", originalCalls, selectedCalls, installations.calls, creator.calls) + if originalCalls != test.originalCalls { + t.Errorf("expected %d original client requests, got %d", test.originalCalls, originalCalls) } - }) - } -} - -func TestLoadConfigInvalidRemoteDiagnostics(t *testing.T) { - content := "remote: remote/config#\n" - client := newRemoteTestClient(t, remoteRoundTripperFunc(func(req *http.Request) (*http.Response, error) { - response := SavedResponse{Status: http.StatusOK} - if req.URL.Path == "/repos/original/.github" { - response.Body = `{"name":".github","default_branch":"main"}` - } else { - response.Body = `{"encoding":"base64","content":"` + base64.StdEncoding.EncodeToString([]byte(content)) + `"}` - } - return response.Response(req), nil - })) - for _, test := range []struct { - name, source string - paths []string - }{ - {name: "local", source: "original/local@main", paths: []string{"app.yml"}}, - {name: "owner default", source: "original/.github@main"}, - } { - t.Run(test.name, func(t *testing.T) { - loader := NewLoader(test.paths, WithOwnerDefault(".github", []string{"app.yml"})) - cfg, err := loader.LoadConfig(context.Background(), client, "original", "local", "main") - if err == nil || err.Error() != "invalid remote value: remote/config#" || cfg.Source != test.source || cfg.Path != "app.yml" || cfg.IsRemote { - t.Fatalf("lost originating file diagnostics: %+v, %v", cfg, err) + if selectedCalls != test.clientCalls { + t.Errorf("expected %d selected client requests, got %d", test.clientCalls, selectedCalls) + } + if installations.calls != 1 { + t.Errorf("expected one installation lookup, got %d", installations.calls) + } + if creator.calls != test.creations { + t.Errorf("expected %d client creations, got %d", test.creations, creator.calls) } }) } From 7339ea52545b3fa7469673287febb1900dab5467 Mon Sep 17 00:00:00 2001 From: rzarkar Date: Fri, 6 Nov 2026 12:10:23 -0500 Subject: [PATCH 4/4] simplify remote loader tests --- appconfig/appconfig_test.go | 66 +++++++++ appconfig/remote_test.go | 261 ------------------------------------ 2 files changed, 66 insertions(+), 261 deletions(-) delete mode 100644 appconfig/remote_test.go diff --git a/appconfig/appconfig_test.go b/appconfig/appconfig_test.go index 2fa9c20a..031fa379 100644 --- a/appconfig/appconfig_test.go +++ b/appconfig/appconfig_test.go @@ -179,6 +179,72 @@ func TestLoadConfig(t *testing.T) { } } +func TestLoadRemoteRef(t *testing.T) { + tests := map[string]struct { + Remote RemoteRef + Expected Config + Error string + }{ + "literalContent": { + Remote: RemoteRef{Remote: "test/remote-ref", Path: ".github/test-app.yml", Ref: TestRef}, + Expected: Config{ + Content: []byte("remote: remote/config\npath: config/test-app.yml\nref: develop\n"), + Source: "test/remote-ref@develop", + Path: ".github/test-app.yml", + IsRemote: true, + }, + }, + "missingFile": { + Remote: RemoteRef{Remote: "test/local-file", Path: ".github/test-app.v2.yml", Ref: TestRef}, + Error: "invalid remote reference: file does not exist", + }, + "defaultBranch": { + Remote: RemoteRef{Remote: "remote/config", Path: "config/test-app.yml"}, + Expected: Config{ + Content: []byte("message: hello\n"), + Source: "remote/config@main", + Path: "config/test-app.yml", + IsRemote: true, + }, + }, + } + + ctx := context.Background() + client := makeTestClient() + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + ld := NewLoader([]string{".github/test-app.v2.yml", ".github/test-app.yml"}) + cfg, err := ld.LoadRemoteRef(ctx, client, TestOwner, test.Remote) + if test.Error != "" { + if err == nil { + t.Fatal("expected error loading config, but got nil") + } + if err.Error() != test.Error { + t.Errorf("incorrect error: expected: %q, actual: %q", test.Error, err) + } + return + } + if err != nil { + t.Fatalf("unexpected error loading config: %v", err) + } + + if test.Expected.Source != cfg.Source { + t.Errorf("incorrect source: expected: %q, actual: %q", test.Expected.Source, cfg.Source) + } + if test.Expected.Path != cfg.Path { + t.Errorf("incorrect path: expected: %q, actual: %q", test.Expected.Path, cfg.Path) + } + if test.Expected.IsRemote != cfg.IsRemote { + t.Errorf("incorrect remote flag: expected: %t, actual: %t", test.Expected.IsRemote, cfg.IsRemote) + } + if !bytes.Equal(test.Expected.Content, cfg.Content) { + t.Errorf("incorrect content\nexpected: %s\n actual: %s", test.Expected.Content, cfg.Content) + } + }) + } +} + func TestLoadConfigWithPrivateRemotes(t *testing.T) { newClient := func(rp *ResponsePlayer) *github.Client { client, _ := github.NewClient(github.WithHTTPClient(&http.Client{Transport: rp})) diff --git a/appconfig/remote_test.go b/appconfig/remote_test.go deleted file mode 100644 index 4c2417a7..00000000 --- a/appconfig/remote_test.go +++ /dev/null @@ -1,261 +0,0 @@ -// Copyright 2026 Palantir Technologies, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package appconfig - -import ( - "context" - "encoding/base64" - "errors" - "net/http" - "reflect" - "testing" - - "github.com/google/go-github/v93/github" - "github.com/palantir/go-githubapp/githubapp" -) - -type remoteRoundTripperFunc func(*http.Request) (*http.Response, error) - -func (f remoteRoundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { - return f(req) -} - -func newRemoteTestClient(t *testing.T, rt http.RoundTripper) *github.Client { - t.Helper() - client, err := github.NewClient(github.WithHTTPClient(&http.Client{Transport: rt})) - if err != nil { - t.Fatal(err) - } - return client -} - -func TestLoadRemoteRefLiteral(t *testing.T) { - content := "remote: remote/config\npath: first.yml\nref: main\n" - var requests []string - client := newRemoteTestClient(t, remoteRoundTripperFunc(func(req *http.Request) (*http.Response, error) { - requests = append(requests, req.URL.RequestURI()) - response := SavedResponse{Status: http.StatusOK, Body: `{"default_branch":"main"}`} - if req.URL.Path != "/repos/remote/config" { - response.Body = `{"encoding":"base64","content":"` + base64.StdEncoding.EncodeToString([]byte(content)) + `"}` - } - return response.Response(req), nil - })) - loader := NewLoader([]string{"first.yml", "second.yml"}, WithRemoteRefParser(func(string, []byte) (*RemoteRef, error) { - t.Error("literal loading invoked the remote parser") - return nil, errors.New("unexpected parser call") - })) - - cfg, err := loader.LoadRemoteRef(context.Background(), client, "original", RemoteRef{Remote: "remote/config"}) - if err != nil { - t.Fatal(err) - } - want := Config{Content: []byte(content), Source: "remote/config@main", Path: "first.yml", IsRemote: true} - if !reflect.DeepEqual(cfg, want) { - t.Errorf("got %+v, want %+v", cfg, want) - } - wantRequests := []string{"/repos/remote/config", "/repos/remote/config/contents/first.yml?ref=main"} - if !reflect.DeepEqual(requests, wantRequests) { - t.Errorf("requests: got %v, want %v", requests, wantRequests) - } -} - -func TestLoadRemoteRefMissingAndEmpty(t *testing.T) { - for name, test := range map[string]struct { - response SavedResponse - missing bool - }{ - "missing target": {response: SavedResponse{Status: http.StatusNotFound}, missing: true}, - "directory": {response: SavedResponse{Status: http.StatusOK, Body: `[]`}, missing: true}, - "empty file": {response: SavedResponse{Status: http.StatusOK, Body: `{"encoding":"base64","content":""}`}}, - } { - t.Run(name, func(t *testing.T) { - calls := 0 - client := newRemoteTestClient(t, remoteRoundTripperFunc(func(req *http.Request) (*http.Response, error) { - calls++ - if req.URL.RequestURI() != "/repos/remote/config/contents/first.yml?ref=main" { - t.Errorf("unexpected request: %s", req.URL) - } - return test.response.Response(req), nil - })) - loader := NewLoader([]string{"first.yml", "second.yml"}) - cfg, err := loader.LoadRemoteRef(context.Background(), client, "original", RemoteRef{Remote: "remote/config", Ref: "main"}) - if test.missing { - if err == nil || err.Error() != "invalid remote reference: file does not exist" { - t.Fatalf("expected missing remote error, got %v", err) - } - } else if err != nil { - t.Fatal(err) - } - if calls != 1 { - t.Errorf("expected one request, got %d", calls) - } - if cfg.IsUndefined() { - t.Error("expected config to retain source metadata") - } - if len(cfg.Content) != 0 { - t.Errorf("expected empty content, got %q", cfg.Content) - } - if cfg.Source != "remote/config@main" { - t.Errorf("incorrect source: %q", cfg.Source) - } - if cfg.Path != "first.yml" { - t.Errorf("incorrect path: %q", cfg.Path) - } - if !cfg.IsRemote { - t.Error("expected remote config") - } - }) - } -} - -func TestLoadRemoteRefInvalidRepository(t *testing.T) { - for _, remote := range []string{"", "owner", "/repo", "owner/"} { - t.Run(remote, func(t *testing.T) { - client := newRemoteTestClient(t, remoteRoundTripperFunc(func(req *http.Request) (*http.Response, error) { - t.Error("invalid remote made an HTTP request") - return (&SavedResponse{Status: http.StatusOK, Body: `{"name":"repo"}`}).Response(req), nil - })) - installations := &testInstallationsService{} - creator := &testClientCreator{client: client} - loader := NewLoader(nil, WithPrivateRemotes(creator, installations)) - cfg, err := loader.LoadRemoteRef(context.Background(), client, "original", RemoteRef{Remote: remote, Path: "app.yml", Ref: "main"}) - if err == nil { - t.Error("expected invalid remote error") - } - if !reflect.DeepEqual(cfg, Config{}) { - t.Errorf("expected empty Config, got %+v", cfg) - } - if installations.calls != 0 { - t.Errorf("invalid remote triggered %d installation lookups", installations.calls) - } - if creator.calls != 0 { - t.Errorf("invalid remote triggered %d client creations", creator.calls) - } - }) - } -} - -func TestLoadRemoteRefSuccessivePrivateFiles(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - original := newRemoteTestClient(t, remoteRoundTripperFunc(func(req *http.Request) (*http.Response, error) { - t.Error("used original client for private remote") - return errorResponse(req, http.StatusForbidden, "denied") - })) - var requests []string - selected := newRemoteTestClient(t, remoteRoundTripperFunc(func(req *http.Request) (*http.Response, error) { - requests = append(requests, req.URL.String()) - if req.Context() != ctx { - t.Error("selected client lost caller context") - } - response := SavedResponse{Status: http.StatusOK} - switch req.URL.Path { - case "/repos/remote/config/contents/child.yml": - response.Body = `{"encoding":"base64","content":"Y2hpbGQ="}` - case "/repos/remote/config/contents/parent.yml": - response.Body = `{"encoding":"none","download_url":"https://download.example/parent"}` - case "/parent": - response.Body = "parent" - } - return response.Response(req), nil - })) - installations := &testInstallationsService{installation: githubapp.Installation{ID: 42}} - creator := &testClientCreator{client: selected} - loader := NewLoader([]string{"unused.yml"}, WithPrivateRemotes(creator, installations)) - - for _, file := range []string{"child", "parent"} { - cfg, err := loader.LoadRemoteRef(ctx, original, "source", RemoteRef{Remote: "remote/config", Path: file + ".yml", Ref: "main"}) - if err != nil { - t.Fatal(err) - } - if string(cfg.Content) != file { - t.Errorf("incorrect content: %q", cfg.Content) - } - if cfg.Source != "remote/config@main" { - t.Errorf("incorrect source: %q", cfg.Source) - } - if cfg.Path != file+".yml" { - t.Errorf("incorrect path: %q", cfg.Path) - } - if !cfg.IsRemote { - t.Error("expected remote config") - } - } - if installations.calls != 2 { - t.Errorf("expected two installation lookups, got %d", installations.calls) - } - if installations.owner != "remote" || installations.repo != "config" { - t.Errorf("incorrect installation repository: %s/%s", installations.owner, installations.repo) - } - if creator.calls != 2 { - t.Errorf("expected two client creations, got %d", creator.calls) - } - if creator.installationID != 42 { - t.Errorf("incorrect installation ID: %d", creator.installationID) - } - wantRequests := []string{ - "https://api.github.com/repos/remote/config/contents/child.yml?ref=main", - "https://api.github.com/repos/remote/config/contents/parent.yml?ref=main", - "https://download.example/parent", - } - if !reflect.DeepEqual(requests, wantRequests) { - t.Errorf("requests: got %v, want %v", requests, wantRequests) - } -} - -func TestLoadRemoteRefAccessFailures(t *testing.T) { - for name, test := range map[string]struct { - lookupErr, creatorErr error - originalStatus int - originalCalls, clientCalls int - creations int - }{ - "lookup failure": {lookupErr: githubapp.InstallationNotFound("remote/config"), originalStatus: http.StatusForbidden, originalCalls: 1}, - "creation failure": {creatorErr: errors.New("creation failed"), originalStatus: http.StatusForbidden, originalCalls: 1, creations: 1}, - "read failure": {originalStatus: http.StatusOK, clientCalls: 1, creations: 1}, - } { - t.Run(name, func(t *testing.T) { - originalCalls, selectedCalls := 0, 0 - original := newRemoteTestClient(t, remoteRoundTripperFunc(func(req *http.Request) (*http.Response, error) { - originalCalls++ - return (&SavedResponse{Status: test.originalStatus, Body: `{"encoding":"base64","content":""}`}).Response(req), nil - })) - selected := newRemoteTestClient(t, remoteRoundTripperFunc(func(req *http.Request) (*http.Response, error) { - selectedCalls++ - return errorResponse(req, http.StatusForbidden, "denied") - })) - installations := &testInstallationsService{installation: githubapp.Installation{ID: 42}, err: test.lookupErr} - creator := &testClientCreator{client: selected, err: test.creatorErr} - loader := NewLoader(nil, WithPrivateRemotes(creator, installations)) - _, err := loader.LoadRemoteRef(context.Background(), original, "source", RemoteRef{Remote: "remote/config", Path: "app.yml", Ref: "main"}) - var responseErr *github.ErrorResponse - if !errors.As(err, &responseErr) || responseErr.Response.StatusCode != http.StatusForbidden { - t.Fatalf("expected access denial, got %v", err) - } - if originalCalls != test.originalCalls { - t.Errorf("expected %d original client requests, got %d", test.originalCalls, originalCalls) - } - if selectedCalls != test.clientCalls { - t.Errorf("expected %d selected client requests, got %d", test.clientCalls, selectedCalls) - } - if installations.calls != 1 { - t.Errorf("expected one installation lookup, got %d", installations.calls) - } - if creator.calls != test.creations { - t.Errorf("expected %d client creations, got %d", test.creations, creator.calls) - } - }) - } -}