-
Notifications
You must be signed in to change notification settings - Fork 16
Make changes to support testing individual commands, add a few tests #15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
57b9ca1
dd20108
c1bb492
ef468eb
ec0883b
8076365
0e91a6e
3122947
6781863
f247ca2
f1d30e0
644fb0d
d236e8f
9a307d6
5d95d27
13dc795
57b6183
a804f54
bd59113
f93fc5e
64bd851
d69e1ae
3738e1f
099849a
0c00848
3ede78e
1b48b0f
09cd4ae
a746559
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,19 +2,27 @@ name: "go-linter" | |
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, reopened] | ||
| merge_group: | ||
| workflow_dispatch: | ||
| push: | ||
| branches: | ||
| - 'main' | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also lint what gets merged into the main branch. |
||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} | ||
| cancel-in-progress: true | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cancel an existing run of the linter for the same commit. |
||
|
|
||
| jobs: | ||
| lint: | ||
| strategy: | ||
| fail-fast: false | ||
| runs-on: ubuntu-latest-xl | ||
| runs-on: ubuntu-latest | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably don't need the XL version. |
||
| env: | ||
| GOPROXY: https://goproxy.githubapp.com/mod,https://proxy.golang.org/,direct | ||
| GOPROXY: https://proxy.golang.org/,direct | ||
| GOPRIVATE: "" | ||
| GONOPROXY: "" | ||
| GONOSUMDB: github.com/github/* | ||
|
|
@@ -24,9 +32,6 @@ jobs: | |
| go-version: ${{ vars.GOVERSION }} | ||
| check-latest: true | ||
| - uses: actions/checkout@v4 | ||
| - name: Configure Go private module access | ||
| run: | | ||
| echo "machine goproxy.githubapp.com login nobody password ${{ secrets.GOPROXY_TOKEN }}" >> $HOME/.netrc | ||
| - name: Lint | ||
| # This also does checkout, setup-go, and proxy setup. | ||
| uses: github/go-linter@v1.2.1 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| name: "Build and test" | ||
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, reopened] | ||
| workflow_dispatch: | ||
| merge_group: | ||
| push: | ||
| branches: | ||
| - 'main' | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| build: | ||
| runs-on: ubuntu-latest | ||
| env: | ||
| GOPROXY: https://proxy.golang.org/,direct | ||
| GOPRIVATE: "" | ||
| GONOPROXY: "" | ||
| GONOSUMDB: github.com/github/* | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: ${{ vars.GOVERSION }} | ||
| check-latest: true | ||
| - name: Verify go.sum is up to date | ||
| run: | | ||
| go mod tidy | ||
| git diff --exit-code go.sum | ||
| if [ $? -ne 0 ]; then | ||
| echo "Error: go.sum has changed, please run `go mod tidy` and commit the result" | ||
| exit 1 | ||
| fi | ||
|
|
||
| - name: Build program | ||
| run: go build -v ./... | ||
|
|
||
| - name: Run tests | ||
| run: go test -race -cover ./... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| package list | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/github/gh-models/internal/azuremodels" | ||
| "github.com/github/gh-models/pkg/command" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestList(t *testing.T) { | ||
| t.Run("NewListCommand happy path", func(t *testing.T) { | ||
| client := azuremodels.NewMockClient() | ||
| modelSummary := &azuremodels.ModelSummary{ | ||
| ID: "test-id-1", | ||
| Name: "test-model-1", | ||
| FriendlyName: "Test Model 1", | ||
| Task: "chat-completion", | ||
| Publisher: "OpenAI", | ||
| Summary: "This is a test model", | ||
| Version: "1.0", | ||
| RegistryName: "azure-openai", | ||
| } | ||
| listModelsCallCount := 0 | ||
| client.MockListModels = func(ctx context.Context) ([]*azuremodels.ModelSummary, error) { | ||
| listModelsCallCount++ | ||
| return []*azuremodels.ModelSummary{modelSummary}, nil | ||
| } | ||
| buf := new(bytes.Buffer) | ||
| cfg := command.NewConfig(buf, buf, client, true, 80) | ||
| listCmd := NewListCommand(cfg) | ||
|
|
||
| _, err := listCmd.ExecuteC() | ||
|
|
||
| require.NoError(t, err) | ||
| require.Equal(t, 1, listModelsCallCount) | ||
| output := buf.String() | ||
| require.Contains(t, output, "Showing 1 available chat models") | ||
| require.Contains(t, output, "DISPLAY NAME") | ||
| require.Contains(t, output, "MODEL NAME") | ||
| require.Contains(t, output, modelSummary.FriendlyName) | ||
| require.Contains(t, output, modelSummary.Name) | ||
| }) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,9 +4,14 @@ package cmd | |
| import ( | ||
| "strings" | ||
|
|
||
| "github.com/cli/go-gh/v2/pkg/auth" | ||
| "github.com/cli/go-gh/v2/pkg/term" | ||
| "github.com/github/gh-models/cmd/list" | ||
| "github.com/github/gh-models/cmd/run" | ||
| "github.com/github/gh-models/cmd/view" | ||
| "github.com/github/gh-models/internal/azuremodels" | ||
| "github.com/github/gh-models/pkg/command" | ||
| "github.com/github/gh-models/pkg/util" | ||
| "github.com/spf13/cobra" | ||
| ) | ||
|
|
||
|
|
@@ -17,9 +22,24 @@ func NewRootCommand() *cobra.Command { | |
| Short: "GitHub Models extension", | ||
| } | ||
|
|
||
| cmd.AddCommand(list.NewListCommand()) | ||
| cmd.AddCommand(run.NewRunCommand()) | ||
| cmd.AddCommand(view.NewViewCommand()) | ||
| terminal := term.FromEnv() | ||
| out := terminal.Out() | ||
| token, _ := auth.TokenForHost("github.com") | ||
|
|
||
| var client azuremodels.Client | ||
|
|
||
| if token == "" { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For I really like that this PR still lets you do some commands without a token, but do you think it's worth trying to preserve the old behavior for
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So the effect feels the same to users. When I try to % ./gh-models run gpt-4o-mini "hello world"
No GitHub token found. Please run 'gh auth login' to authenticate.
Error: not authenticated
Usage:
gh models run [model] [prompt] [flags]
Flags:
-h, --help help for run
--max-tokens string Limit the maximum tokens for the model response.
--system-prompt string Prompt the system.
--temperature string Controls randomness in the response, use lower to be more deterministic.
--top-p string Controls text diversity by selecting the most probable words until a set probability is reached.Are you worried about us trying to make an HTTP call with an invalid (blank) token?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't really mind (actually making the HTTP call is not a problem), I was just a little worried that the error would be ugly. Given that it isn't, I have no qualms here - no need to follow up in another branch. |
||
| util.WriteToOut(out, "No GitHub token found. Please run 'gh auth login' to authenticate.\n") | ||
| client = azuremodels.NewUnauthenticatedClient() | ||
| } else { | ||
| client = azuremodels.NewAzureClient(token) | ||
| } | ||
|
|
||
| cfg := command.NewConfigWithTerminal(terminal, client) | ||
|
|
||
| cmd.AddCommand(list.NewListCommand(cfg)) | ||
| cmd.AddCommand(run.NewRunCommand(cfg)) | ||
| cmd.AddCommand(view.NewViewCommand(cfg)) | ||
|
|
||
| // Cobra doesn't have a way to specify a two word command (ie. "gh models"), so set a custom usage template | ||
| // with `gh`` in it. Cobra will use this template for the root and all child commands. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run the linter on fewer PR events, such as when the PR is labelled.