Skip to content

Conversation

@mikeland73
Copy link
Contributor

Summary

This PR does a few things:

  • Adds support for github token. This prevents rate-limiting when getting releases.json
  • Adds resolve to API. This will allow devbox to validate packages exist and resolve versions in order to use lockfile.

@gcurtis I'm copying the devbox structure for the public interface (RunX interface) but was curious if you would do it differently. Specifically, we could remove the public interface and rename impl to just be runx and use a struct:

runx.New(runx.Opts{
  GithubAPIToken: "",
}).Install(...)

thoughts?

How was it tested?

RUNX_GITHUB_API_TOKEN=<token> runx +jetpack-io/envsec envsec
RUNX_GITHUB_API_TOKEN=<token> pkg resolve jetpack-io/envsec@latest
RUNX_GITHUB_API_TOKEN=<token> pkg resolve jetpack-io/envsec@v0.0.3

@mikeland73 mikeland73 requested review from gcurtis and loreto October 5, 2023 20:07
Copy link
Contributor

@gcurtis gcurtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I think we should eschew the interface and just use a struct. The impl pattern in Devbox feels a bit awkward and led to a giant interface.

Comment on lines 5 to 7
type RunX struct {
GithubAPIToken string
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference would be to put this struct directly in the runx package and remove the interface. Because the GitHub API token is optional, I'd also remove runopt in favor of setting the GithubAPIToken field directly. For example:

var r runx.Runx
// -- or --
// r := runx.RunX {
//     GithubAPIToken: "token",
// }
r.Run(ctx, "golangci-lint", "run")

func NewLocalRegistry(ctx context.Context, githubAPIToken string) (*Registry, error) {
cacheDir, err := os.UserCacheDir()
if err != nil {
cacheDir = "~/.cache"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ~ won't expand if the path is used outside a shell. You probably want:

Suggested change
cacheDir = "~/.cache"
cacheDir = os.Expandenv("$HOME/.cache")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os.UserCacheDir() usually only errors if $HOME is empty. Will return error instead .

@mikeland73 mikeland73 merged commit e283f10 into main Oct 6, 2023
@mikeland73 mikeland73 deleted the landau/runx-improvements branch October 6, 2023 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants