From 726e440fcb935948430fd8ff9b2115a5846b8057 Mon Sep 17 00:00:00 2001 From: Billy Keyes Date: Fri, 1 May 2026 15:58:28 -0400 Subject: [PATCH 1/3] Simplify file loading in appconfig package In an older version of the GitHub API, getting the contents of a large file returned a 422 response. This is no longer the case. Instead, you get back a standard JSON response with an empty content field. Simplify the logic for loading config by making a single call to the contents API, then using the response content to determine if we need to use the direct download URL or not. I verified the API behavior against GitHub Enterprise 3.17, so I believe it will be the same on GitHub.com (this also matches the API docs.) I discovered this as part of trying to upgrade to go-github v85, which refactored the DownloadContents function in a way that broke our tests. --- appconfig/appconfig.go | 64 ++++++++----------- appconfig/appconfig_test.go | 1 - .../testdata/local-file-large-contents.yml | 12 ++-- .../local-file-large-dir-contents.yml | 10 --- 4 files changed, 35 insertions(+), 52 deletions(-) delete mode 100644 appconfig/testdata/local-file-large-dir-contents.yml diff --git a/appconfig/appconfig.go b/appconfig/appconfig.go index dbdbf028e..c09c2ac0d 100644 --- a/appconfig/appconfig.go +++ b/appconfig/appconfig.go @@ -267,56 +267,57 @@ func (ld *Loader) loadDefaultConfig(ctx context.Context, client *github.Client, // getFileContents returns the content of the file at path on ref in owner/repo // if it exists. Returns an empty slice and false if the file does not exist. func getFileContents(ctx context.Context, client *github.Client, owner, repo, ref, path string) ([]byte, bool, error) { + // GetContents returns encoded content for files < 1MB and a download URL + // for files between 1MB and 100MB. It returns an error for files >100MB, + // but if an app has a configuration file that large, there are probably + // other issues... file, _, _, err := client.Repositories.GetContents(ctx, owner, repo, path, &github.RepositoryContentGetOptions{ Ref: ref, }) if err != nil { - switch { - case isNotFound(err): + if isNotFound(err) { return nil, false, nil - case isTooLargeError(err): - b, err := getLargeFileContents(ctx, client, owner, repo, ref, path) - return b, true, err } return nil, false, errors.Wrap(err, "failed to read file") } - // file will be nil if the path exists but is a directory + // The file will be nil if the path exists but is a directory if file == nil { return nil, false, nil } + // Ignore the decoding error and try the download URL. The most likely + // reason is that the file is larger than 1MB. content, err := file.GetContent() - if err != nil { - return nil, true, errors.Wrap(err, "failed to decode file content") + if err == nil && content != "" { + return []byte(content), true, nil } - return []byte(content), true, nil -} + downloadURL := file.GetDownloadURL() + if downloadURL == "" { + return nil, false, errors.New("download url is empty") + } -// getLargeFileContents is similar to getFileContents, but works for files up -// to 100MB. Unlike getFileContents, it returns an error if the file does not -// exist. -func getLargeFileContents(ctx context.Context, client *github.Client, owner, repo, ref, path string) ([]byte, error) { - body, res, err := client.Repositories.DownloadContents(ctx, owner, repo, path, &github.RepositoryContentGetOptions{ - Ref: ref, - }) + req, err := http.NewRequestWithContext(ctx, "GET", downloadURL, nil) if err != nil { - return nil, errors.Wrap(err, "failed to read file") + return nil, false, errors.Wrap(err, "failed to create download request") } - defer func() { - _ = body.Close() - }() - if res.StatusCode != http.StatusOK { - return nil, errors.Errorf("failed to read file: unexpected status code %d", res.StatusCode) + res, err := client.BareDo(ctx, req) + if err != nil { + return nil, false, errors.Wrap(err, "failed to download file") } - b, err := io.ReadAll(body) + defer func() { + _, _ = io.Copy(io.Discard, res.Body) + _ = res.Body.Close() + }() + + b, err := io.ReadAll(res.Body) if err != nil { - return nil, errors.Wrap(err, "failed to read file") + return nil, false, errors.Wrap(err, "failed to read file") } - return b, nil + return b, true, nil } func isNotFound(err error) bool { @@ -325,14 +326,3 @@ func isNotFound(err error) bool { } return false } - -func isTooLargeError(err error) bool { - if rerr, ok := err.(*github.ErrorResponse); ok { - for _, err := range rerr.Errors { - if err.Code == "too_large" { - return true - } - } - } - return false -} diff --git a/appconfig/appconfig_test.go b/appconfig/appconfig_test.go index 84eb77ad4..0ea8e29f6 100644 --- a/appconfig/appconfig_test.go +++ b/appconfig/appconfig_test.go @@ -149,7 +149,6 @@ func makeTestClient() *github.Client { "/repos/test/local-file/contents/.github/test-app.v2.yml": "404.yml", "/repos/test/local-file-large/contents/.github/test-app.yml": "local-file-large-contents.yml", - "/repos/test/local-file-large/contents/.github": "local-file-large-dir-contents.yml", "/test/local-file-large/develop/.github/test-app.yml": "local-file-large-download.yml", "/repos/test/remote-ref/contents/.github/test-app.yml": "remote-ref-contents.yml", diff --git a/appconfig/testdata/local-file-large-contents.yml b/appconfig/testdata/local-file-large-contents.yml index 3373cfe4c..97f1346d8 100644 --- a/appconfig/testdata/local-file-large-contents.yml +++ b/appconfig/testdata/local-file-large-contents.yml @@ -1,7 +1,11 @@ -- status: 422 +- status: 200 body: | { - "errors": [ - {"code": "too_large"} - ] + "name": "test-app.yml", + "path": ".github/test-app.yml", + "size": 15000000, + "download_url": "https://raw.githubusercontent.com/test/local-file-large/develop/.github/test-app.yml?token=downloadtoken", + "type": "file", + "content": "", + "encoding": "none" } diff --git a/appconfig/testdata/local-file-large-dir-contents.yml b/appconfig/testdata/local-file-large-dir-contents.yml deleted file mode 100644 index bf75a2275..000000000 --- a/appconfig/testdata/local-file-large-dir-contents.yml +++ /dev/null @@ -1,10 +0,0 @@ -- status: 200 - body: | - [ - { - "type": "file", - "name": "test-app.yml", - "path": ".github/test-app.yml", - "download_url": "https://raw.githubusercontent.com/test/local-file-large/develop/.github/test-app.yml" - } - ] From a6b7c5be0f9151af58ec82851ec295d77b1584d8 Mon Sep 17 00:00:00 2001 From: Billy Keyes Date: Fri, 1 May 2026 16:19:37 -0400 Subject: [PATCH 2/3] Use a plain HTTP client, handle empty files --- appconfig/appconfig.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/appconfig/appconfig.go b/appconfig/appconfig.go index c09c2ac0d..5ad48cd80 100644 --- a/appconfig/appconfig.go +++ b/appconfig/appconfig.go @@ -286,10 +286,10 @@ func getFileContents(ctx context.Context, client *github.Client, owner, repo, re return nil, false, nil } - // Ignore the decoding error and try the download URL. The most likely - // reason is that the file is larger than 1MB. + // If decoding the content fails, ignore the error and try the download URL + // instead. The most likely error is if the file is larger than 1MB. content, err := file.GetContent() - if err == nil && content != "" { + if err == nil { return []byte(content), true, nil } @@ -303,7 +303,7 @@ func getFileContents(ctx context.Context, client *github.Client, owner, repo, re return nil, false, errors.Wrap(err, "failed to create download request") } - res, err := client.BareDo(ctx, req) + res, err := client.Client().Do(req) if err != nil { return nil, false, errors.Wrap(err, "failed to download file") } @@ -313,6 +313,10 @@ func getFileContents(ctx context.Context, client *github.Client, owner, repo, re _ = res.Body.Close() }() + if res.StatusCode != http.StatusOK { + return nil, false, errors.Errorf("failed to download file: unexpected status code %d", res.StatusCode) + } + b, err := io.ReadAll(res.Body) if err != nil { return nil, false, errors.Wrap(err, "failed to read file") From f62e00ba1f00867e6a088256a5782c7d2e645249 Mon Sep 17 00:00:00 2001 From: Billy Keyes Date: Fri, 1 May 2026 16:22:28 -0400 Subject: [PATCH 3/3] Return true on error if we know the file exists --- appconfig/appconfig.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/appconfig/appconfig.go b/appconfig/appconfig.go index 5ad48cd80..8820cf882 100644 --- a/appconfig/appconfig.go +++ b/appconfig/appconfig.go @@ -295,17 +295,17 @@ func getFileContents(ctx context.Context, client *github.Client, owner, repo, re downloadURL := file.GetDownloadURL() if downloadURL == "" { - return nil, false, errors.New("download url is empty") + return nil, true, errors.New("download url is empty") } req, err := http.NewRequestWithContext(ctx, "GET", downloadURL, nil) if err != nil { - return nil, false, errors.Wrap(err, "failed to create download request") + return nil, true, errors.Wrap(err, "failed to create download request") } res, err := client.Client().Do(req) if err != nil { - return nil, false, errors.Wrap(err, "failed to download file") + return nil, true, errors.Wrap(err, "failed to download file") } defer func() { @@ -314,12 +314,12 @@ func getFileContents(ctx context.Context, client *github.Client, owner, repo, re }() if res.StatusCode != http.StatusOK { - return nil, false, errors.Errorf("failed to download file: unexpected status code %d", res.StatusCode) + return nil, true, errors.Errorf("failed to download file: unexpected status code %d", res.StatusCode) } b, err := io.ReadAll(res.Body) if err != nil { - return nil, false, errors.Wrap(err, "failed to read file") + return nil, true, errors.Wrap(err, "failed to read file") } return b, true, nil }