Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for loading SNMP config (and MIB profiles) from git-backed sources, including optional GitHub token auth via KT_GITHUB_ACCESS_TOKEN.
Changes:
- Add
git://scheme handling to SNMP utilLoadFile/WriteFile. - Add BasicAuth support for cloning MIB profiles from git when a token is present.
- Update Go module dependencies (notably introducing go-git/v6 and related transitive bumps).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| pkg/inputs/snmp/util/file.go | Adds git scheme support for config load/write via cloning a repo and reading a file. |
| pkg/inputs/snmp/mibs/git.go | Adds token-based auth for cloning profile repositories. |
| go.mod | Adds go-git/v6 and updates several dependency versions. |
| go.sum | Updates dependency checksums in line with go.mod changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| switch u.Scheme { | ||
| case "http", "https": | ||
| return loadFromHttp(ctx, file) | ||
| case "s3": | ||
| return loadFromS3(ctx, u, getS3Downloader()) | ||
| case "s3m": | ||
| return loadFromS3(ctx, u, getMockS3Client()) | ||
| case "git": | ||
| return loadFromGit(ctx, u) | ||
| default: | ||
| return ioutil.ReadFile(file) | ||
| } |
There was a problem hiding this comment.
Adding git support introduces new behavior in LoadFile/WriteFile, but the existing unit tests in file_test.go don’t cover the git:// scheme. Please add tests covering at least the URL parsing behavior and the expected write semantics (read-only error vs. commit).
|
|
||
| "github.com/go-git/go-git/v5" | ||
| "github.com/go-git/go-git/v5/plumbing" | ||
| githttp "github.com/go-git/go-git/v6/plumbing/transport/http" |
There was a problem hiding this comment.
This file uses go-git/v5 (git.PlainCloneContext) but imports plumbing/transport/http from go-git/v6. The BasicAuth type won’t satisfy the v5 transport.AuthMethod interface due to different module paths, so this will not compile. Import the v5 transport/http package (or migrate fully to v6).
| githttp "github.com/go-git/go-git/v6/plumbing/transport/http" | |
| githttp "github.com/go-git/go-git/v5/plumbing/transport/http" |
| github.com/elliotchance/orderedmap v1.4.0 | ||
| github.com/fsnotify/fsnotify v1.6.0 | ||
| github.com/go-git/go-git/v5 v5.16.5 | ||
| github.com/go-git/go-git/v6 v6.0.0-20260216160506-e6a3f881772f |
There was a problem hiding this comment.
github.com/go-git/go-git/v6 is added as a direct dependency, but the code in this PR still uses go-git/v5 APIs. Mixing v5 and v6 will either fail to compile (auth types) or bloat deps; prefer sticking to v5 everywhere here or doing a dedicated migration PR to v6.
| github.com/go-git/go-git/v6 v6.0.0-20260216160506-e6a3f881772f |
| file := path.Join(dir, filePath) | ||
| return ioutil.ReadFile(file) |
There was a problem hiding this comment.
filePath comes from the URL and is joined directly; values like ../.. can escape the temp clone directory and read arbitrary files. Use a secure join (e.g. filepath-securejoin) or clean+validate the path stays within dir before reading.
| _, err = git.PlainCloneContext(ctx, dir, false, &git.CloneOptions{ | ||
| URL: gitRepo, | ||
| Auth: auth, | ||
| Progress: os.Stdout, |
There was a problem hiding this comment.
Cloning with Progress: os.Stdout will emit noisy output in library/daemon contexts and can interfere with CLI output. Prefer wiring this to your logger, making it configurable, or using io.Discard by default.
| Progress: os.Stdout, | |
| Progress: io.Discard, |
| "github.com/aws/aws-sdk-go/service/s3" | ||
| "github.com/aws/aws-sdk-go/service/s3/s3manager" | ||
| "github.com/go-git/go-git/v5" | ||
| githttp "github.com/go-git/go-git/v6/plumbing/transport/http" |
There was a problem hiding this comment.
git.CloneOptions.Auth comes from go-git/v5, but this file imports plumbing/transport/http from go-git/v6. Those types are not assignment-compatible, so this won’t compile. Use the v5 plumbing/transport/http package (or upgrade all go-git imports consistently to v6).
| githttp "github.com/go-git/go-git/v6/plumbing/transport/http" | |
| githttp "github.com/go-git/go-git/v5/plumbing/transport/http" |
| // noop, assume that file is not to be changed. | ||
| // @todo, allow a commit after disco? | ||
| func writeToGit(ctx context.Context, url *url.URL, payload []byte) error { | ||
| return nil |
There was a problem hiding this comment.
writeToGit is wired into WriteFile for the git:// scheme but always returns nil without persisting anything. This makes callers believe the config was saved when it wasn’t; return an explicit error (e.g. “git scheme is read-only”) or implement the write+commit behavior.
| return nil | |
| return fmt.Errorf("git scheme is read-only: cannot write to %s", url.String()) |
| pts := strings.Split(url.String(), "/") | ||
| gitRepo := "" | ||
| filePath := "" | ||
| if len(pts) >= 6 { | ||
| gitRepo = "https://" + strings.Join(pts[2:5], "/") + ".git" | ||
| filePath = strings.Join(pts[5:], "/") | ||
| } else { | ||
| return nil, fmt.Errorf("Invalid git url path: %s", url.String()) | ||
| } |
There was a problem hiding this comment.
Git URL parsing here doesn’t match the PR description example (git://Users/... local path). With the current split logic this becomes https://Users/pye/src.git, which is invalid for a local repo. Consider using url.Host + url.Path (or supporting git:///absolute/path/... for local), and document the expected git URL format.
1 participant
KT_GITHUB_ACCESS_TOKEN=$PERSONAL_ACCESS_TOKEN ./bin/ktranslate -snmp git://Users/pye/src/configtest/foo/snmp.yamlAllow a system like this where the snmp.yaml file is pulled from a remote git repo. No need to have
KT_GITHUB_ACCESS_TOKENdefined if not needed.Also allows
KT_GITHUB_ACCESS_TOKENto be defined when pulling snmp profiles from a private repo.Closes #875 #869