-
Notifications
You must be signed in to change notification settings - Fork 76
Render console instructions instead of editing existing MCP/setup files in init command #13769
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
caaa46f
bc34540
7def694
201da67
503f453
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 | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -137,8 +137,9 @@ func upgradeCopilotSetupSteps(verbose bool, actionMode workflow.ActionMode, vers | |||||||||||||
| return ensureCopilotSetupStepsWithUpgrade(verbose, actionMode, version, true) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // ensureCopilotSetupStepsWithUpgrade creates or updates .github/workflows/copilot-setup-steps.yml | ||||||||||||||
| // When upgradeVersion is true, it will update existing actions/setup-cli versions | ||||||||||||||
| // ensureCopilotSetupStepsWithUpgrade creates .github/workflows/copilot-setup-steps.yml | ||||||||||||||
| // If the file already exists, it renders console instructions instead of editing | ||||||||||||||
| // When upgradeVersion is true and called from upgrade command, this is a special case | ||||||||||||||
| func ensureCopilotSetupStepsWithUpgrade(verbose bool, actionMode workflow.ActionMode, version string, upgradeVersion bool) error { | ||||||||||||||
| copilotSetupLog.Printf("Creating copilot-setup-steps.yml with action mode: %s, version: %s, upgradeVersion: %v", actionMode, version, upgradeVersion) | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -168,9 +169,10 @@ func ensureCopilotSetupStepsWithUpgrade(verbose bool, actionMode workflow.Action | |||||||||||||
| (strings.Contains(contentStr, "Install gh-aw extension") && strings.Contains(contentStr, "curl -fsSL")) | ||||||||||||||
| hasActionInstall := strings.Contains(contentStr, "actions/setup-cli") | ||||||||||||||
|
|
||||||||||||||
| // If we have an install step and upgradeVersion is true, attempt to upgrade the version | ||||||||||||||
| // If we have an install step and upgradeVersion is true, this is from upgrade command | ||||||||||||||
| // In this case, we still update the file for backward compatibility | ||||||||||||||
| if (hasLegacyInstall || hasActionInstall) && upgradeVersion { | ||||||||||||||
| copilotSetupLog.Print("Extension install step exists, attempting version upgrade") | ||||||||||||||
| copilotSetupLog.Print("Extension install step exists, attempting version upgrade (upgrade command)") | ||||||||||||||
|
|
||||||||||||||
| // Parse existing workflow | ||||||||||||||
| var workflow Workflow | ||||||||||||||
|
|
@@ -209,43 +211,22 @@ func ensureCopilotSetupStepsWithUpgrade(verbose bool, actionMode workflow.Action | |||||||||||||
| return nil | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // File exists - render instructions instead of editing | ||||||||||||||
| if hasLegacyInstall || hasActionInstall { | ||||||||||||||
| copilotSetupLog.Print("Extension install step already exists, skipping update") | ||||||||||||||
| copilotSetupLog.Print("Extension install step already exists, file is up to date") | ||||||||||||||
| if verbose { | ||||||||||||||
| fmt.Fprintf(os.Stderr, "Skipping %s (already has gh-aw extension install step)\n", setupStepsPath) | ||||||||||||||
|
Comment on lines
+216
to
218
|
||||||||||||||
| copilotSetupLog.Print("Extension install step already exists, file is up to date") | |
| if verbose { | |
| fmt.Fprintf(os.Stderr, "Skipping %s (already has gh-aw extension install step)\n", setupStepsPath) | |
| copilotSetupLog.Print("Extension install step detected; leaving file unchanged") | |
| if verbose { | |
| fmt.Fprintf(os.Stderr, "Skipping %s (detected existing gh-aw extension install step)\n", setupStepsPath) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,7 +70,7 @@ func TestEnsureCopilotSetupSteps(t *testing.T) { | |
| }, | ||
| }, | ||
| { | ||
| name: "injects extension install into existing workflow", | ||
| name: "renders instructions for existing workflow without install step", | ||
| existingWorkflow: &Workflow{ | ||
| Name: "Copilot Setup Steps", | ||
| On: "workflow_dispatch", | ||
|
|
@@ -93,7 +93,7 @@ func TestEnsureCopilotSetupSteps(t *testing.T) { | |
| verbose: false, | ||
| wantErr: false, | ||
| validateContent: func(t *testing.T, content []byte) { | ||
|
Comment on lines
73
to
95
|
||
| // Unmarshal YAML content into Workflow struct for structured validation | ||
| // File should NOT be modified - should remain with only 2 steps | ||
| var wf Workflow | ||
| if err := yaml.Unmarshal(content, &wf); err != nil { | ||
| t.Fatalf("Failed to unmarshal workflow YAML: %v", err) | ||
|
|
@@ -103,13 +103,14 @@ func TestEnsureCopilotSetupSteps(t *testing.T) { | |
| t.Fatalf("Expected job 'copilot-setup-steps' not found") | ||
| } | ||
|
|
||
| // Extension install and verify steps should be injected at the beginning | ||
| if len(job.Steps) < 3 { | ||
| t.Fatalf("Expected at least 3 steps after injection (1 injected + 2 existing), got %d", len(job.Steps)) | ||
| // File should remain unchanged with only 2 existing steps | ||
| if len(job.Steps) != 2 { | ||
| t.Errorf("Expected 2 steps (file should not be modified), got %d", len(job.Steps)) | ||
| } | ||
|
|
||
| if job.Steps[0].Name != "Install gh-aw extension" { | ||
| t.Errorf("Expected first step to be 'Install gh-aw extension', got %q", job.Steps[0].Name) | ||
| // Verify the install step was NOT injected | ||
| if job.Steps[0].Name == "Install gh-aw extension" { | ||
| t.Errorf("Expected 'Install gh-aw extension' step to NOT be injected (instructions should be rendered)") | ||
| } | ||
| }, | ||
| }, | ||
|
|
@@ -729,34 +730,32 @@ jobs: | |
| t.Fatalf("Failed to write existing workflow: %v", err) | ||
| } | ||
|
|
||
| // Update with release mode | ||
| // Call with release mode - should render instructions instead of modifying | ||
| testVersion := "v3.0.0" | ||
| err = ensureCopilotSetupSteps(false, workflow.ActionModeRelease, testVersion) | ||
| if err != nil { | ||
| t.Fatalf("ensureCopilotSetupSteps() failed: %v", err) | ||
| } | ||
|
|
||
| // Read updated file | ||
| // Read file - should remain unchanged | ||
| content, err := os.ReadFile(setupStepsPath) | ||
| if err != nil { | ||
| t.Fatalf("Failed to read updated file: %v", err) | ||
| t.Fatalf("Failed to read file: %v", err) | ||
| } | ||
|
|
||
| contentStr := string(content) | ||
|
|
||
| // Verify release mode injection | ||
| if !strings.Contains(contentStr, "actions/setup-cli@v3.0.0") { | ||
| t.Errorf("Expected injected action with @v3.0.0 tag, got:\n%s", contentStr) | ||
| // Verify file was NOT modified - should remain identical to existingContent | ||
| if contentStr != existingContent { | ||
| t.Errorf("Expected file to remain unchanged (instructions should be rendered instead), got:\n%s", contentStr) | ||
| } | ||
| if !strings.Contains(contentStr, "version: v3.0.0") { | ||
| t.Errorf("Expected version: v3.0.0 parameter, got:\n%s", contentStr) | ||
| } | ||
| if !strings.Contains(contentStr, "actions/checkout@v4") { | ||
| t.Errorf("Expected checkout step to be injected") | ||
|
|
||
| // Verify the install step was NOT injected | ||
| if strings.Contains(contentStr, "actions/setup-cli") { | ||
| t.Errorf("Expected 'actions/setup-cli' to NOT be injected (instructions should be rendered)") | ||
| } | ||
| // Verify original step is preserved | ||
| if !strings.Contains(contentStr, "Some other step") { | ||
| t.Errorf("Expected original step to be preserved") | ||
| if strings.Contains(contentStr, "Install gh-aw extension") { | ||
| t.Errorf("Expected 'Install gh-aw extension' step to NOT be injected (instructions should be rendered)") | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -796,26 +795,31 @@ jobs: | |
| t.Fatalf("Failed to write existing workflow: %v", err) | ||
| } | ||
|
|
||
| // Update with dev mode | ||
| // Call with dev mode - should render instructions instead of modifying | ||
| err = ensureCopilotSetupSteps(false, workflow.ActionModeDev, "dev") | ||
| if err != nil { | ||
| t.Fatalf("ensureCopilotSetupSteps() failed: %v", err) | ||
| } | ||
|
|
||
| // Read updated file | ||
| // Read file - should remain unchanged | ||
| content, err := os.ReadFile(setupStepsPath) | ||
| if err != nil { | ||
| t.Fatalf("Failed to read updated file: %v", err) | ||
| t.Fatalf("Failed to read file: %v", err) | ||
| } | ||
|
|
||
| contentStr := string(content) | ||
|
|
||
| // Verify dev mode injection | ||
| if !strings.Contains(contentStr, "curl -fsSL") { | ||
| t.Errorf("Expected curl command in dev mode") | ||
| // Verify file was NOT modified - should remain identical to existingContent | ||
| if contentStr != existingContent { | ||
| t.Errorf("Expected file to remain unchanged (instructions should be rendered instead), got:\n%s", contentStr) | ||
| } | ||
| if !strings.Contains(contentStr, "install-gh-aw.sh") { | ||
| t.Errorf("Expected install-gh-aw.sh in dev mode") | ||
|
|
||
| // Verify the install step was NOT injected | ||
| if strings.Contains(contentStr, "curl -fsSL") { | ||
| t.Errorf("Expected 'curl' command to NOT be injected (instructions should be rendered)") | ||
| } | ||
| if strings.Contains(contentStr, "install-gh-aw.sh") { | ||
| t.Errorf("Expected 'install-gh-aw.sh' to NOT be injected (instructions should be rendered)") | ||
| } | ||
| if strings.Contains(contentStr, "actions/setup-cli") { | ||
| t.Errorf("Did not expect actions/setup-cli in dev mode") | ||
|
|
||
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.
In
ensureCopilotSetupStepsWithUpgrade, theos.Stat(setupStepsPath)existence check later in this function treats any non-nil error as "file doesn't exist" and will fall through toos.WriteFile(...). IfStatfails for reasons other thanErrNotExist(permissions/IO), this can overwrite an existing workflow—contradicting the new "don’t edit existing files" init behavior. Recommend handlingErrNotExistexplicitly and returning an error on otherStatfailures.