From a9867ade57602f565cc7d6203b965b1c8186b2cf Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 7 Feb 2026 10:08:44 +0000 Subject: [PATCH 1/4] feat: Add Perforce (Helix Core) VCS support Introduce a VCS provider abstraction layer and implement Perforce support alongside the existing Git integration. This enables the CLI to work with Perforce workspaces for branch detection, local changes, and personal builds. Changes: - VCS provider interface (VCSProvider) with Git and Perforce implementations - Auto-detection of VCS type (Git repo vs Perforce workspace) - VCS root CRUD API methods (create, read, list, delete, attach) - Perforce-aware branch/revision formatting in build triggers and change display - Support for `--local-changes p4` to upload Perforce pending changelists - Integration tests with p4d container for VCS root operations - Unit tests for all VCS providers and API methods https://claude.ai/code/session_01Buy3ZEvVTWwhkQHU4H442q --- internal/api/builds.go | 8 +- internal/api/interface.go | 8 + internal/api/perforce_test.go | 358 ++++++++++++++++++++++++++++++ internal/api/vcsroots.go | 167 ++++++++++++++ internal/api/vcsroots_test.go | 347 +++++++++++++++++++++++++++++ internal/cmd/run_analysis.go | 31 ++- internal/cmd/run_lifecycle.go | 78 +++++-- internal/cmd/testutil_test.go | 31 +++ internal/cmd/vcs_git.go | 63 ++++++ internal/cmd/vcs_perforce.go | 238 ++++++++++++++++++++ internal/cmd/vcs_provider.go | 71 ++++++ internal/cmd/vcs_provider_test.go | 198 +++++++++++++++++ 12 files changed, 1564 insertions(+), 34 deletions(-) create mode 100644 internal/api/perforce_test.go create mode 100644 internal/api/vcsroots.go create mode 100644 internal/api/vcsroots_test.go create mode 100644 internal/cmd/vcs_git.go create mode 100644 internal/cmd/vcs_perforce.go create mode 100644 internal/cmd/vcs_provider.go create mode 100644 internal/cmd/vcs_provider_test.go diff --git a/internal/api/builds.go b/internal/api/builds.go index f7000a6..f7cb297 100644 --- a/internal/api/builds.go +++ b/internal/api/builds.go @@ -120,7 +120,8 @@ type RunBuildOptions struct { AgentID int Tags []string PersonalChangeID string - Revision string // Base revision (commit SHA) for personal builds + Revision string // Base revision (commit SHA or changelist number) for personal builds + VCSBranchFormatter func(string) string `json:"-"` // Optional: converts branch name to VCS-specific ref format } // RunBuild runs a new build with full options @@ -184,7 +185,10 @@ func (c *Client) RunBuild(buildTypeID string, opts RunBuildOptions) (*Build, err if opts.Revision != "" { vcsBranch := opts.Branch - if vcsBranch != "" && !strings.HasPrefix(vcsBranch, "refs/") { + if opts.VCSBranchFormatter != nil { + vcsBranch = opts.VCSBranchFormatter(vcsBranch) + } else if vcsBranch != "" && !strings.HasPrefix(vcsBranch, "refs/") && !strings.HasPrefix(vcsBranch, "//") { + // Default: assume Git-style refs unless it looks like a Perforce depot path vcsBranch = "refs/heads/" + vcsBranch } req.Revisions = &Revisions{ diff --git a/internal/api/interface.go b/internal/api/interface.go index fca9a09..8238b5e 100644 --- a/internal/api/interface.go +++ b/internal/api/interface.go @@ -102,6 +102,14 @@ type ClientInterface interface { RemoveProjectFromPool(poolID int, projectID string) error SetAgentPool(agentID int, poolID int) error + // VCS Roots + GetVcsRoots(opts VcsRootOptions) (*VcsRootList, error) + GetVcsRoot(id string) (*VcsRoot, error) + CreateVcsRoot(req CreateVcsRootRequest) (*VcsRoot, error) + DeleteVcsRoot(id string) error + VcsRootExists(id string) bool + AttachVcsRoot(buildTypeID string, vcsRootID string) error + // Raw API access RawRequest(method, path string, body io.Reader, headers map[string]string) (*RawResponse, error) } diff --git a/internal/api/perforce_test.go b/internal/api/perforce_test.go new file mode 100644 index 0000000..bfe4819 --- /dev/null +++ b/internal/api/perforce_test.go @@ -0,0 +1,358 @@ +//go:build integration + +package api_test + +import ( + "context" + "fmt" + "log" + "strings" + "testing" + "time" + + "github.com/JetBrains/teamcity-cli/internal/api" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/testcontainers/testcontainers-go" + "github.com/testcontainers/testcontainers-go/wait" +) + +const ( + p4dImage = "sourcegraph/helix-p4d:latest" + p4dName = "tc-test-p4d" +) + +// perforceTestEnv holds the state for Perforce integration tests +type perforceTestEnv struct { + container testcontainers.Container + port string + host string + ctx context.Context + vcsRootID string + configID string +} + +func (e *perforceTestEnv) Cleanup() { + if e.container != nil { + _ = e.container.Terminate(e.ctx) + } +} + +// startP4D starts a Perforce server container and populates it with test data. +func startP4D(ctx context.Context, networkName string) (*perforceTestEnv, error) { + log.Println("Starting Perforce server (p4d)...") + + aliases := map[string][]string{} + var networks []string + if networkName != "" { + networks = []string{networkName} + aliases[networkName] = []string{"perforce-server"} + } + + container, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{ + ContainerRequest: testcontainers.ContainerRequest{ + Name: p4dName, + Image: p4dImage, + ExposedPorts: []string{"1666/tcp"}, + Networks: networks, + NetworkAliases: aliases, + WaitingFor: wait.ForLog("p4d -r"). + WithStartupTimeout(2 * time.Minute), + }, + Started: true, + }) + if err != nil { + return nil, fmt.Errorf("start p4d: %w", err) + } + + host, _ := container.Host(ctx) + port, _ := container.MappedPort(ctx, "1666/tcp") + + env := &perforceTestEnv{ + container: container, + host: host, + port: port.Port(), + ctx: ctx, + } + + log.Printf("P4D running at %s:%s", host, env.port) + + // Give p4d a moment to fully initialize + time.Sleep(3 * time.Second) + + // Populate with test depot content + if err := populateP4Depot(ctx, container); err != nil { + env.Cleanup() + return nil, fmt.Errorf("populate depot: %w", err) + } + + return env, nil +} + +// populateP4Depot creates a test depot with files in the p4d container. +func populateP4Depot(ctx context.Context, container testcontainers.Container) error { + commands := [][]string{ + // Create a client workspace for initial setup + {"bash", "-c", `p4 -p localhost:1666 -u admin client -o test-setup | +sed 's|//depot/...|//depot/main/...|' | +p4 -p localhost:1666 -u admin client -i`}, + // Create the depot structure with a test file + {"bash", "-c", `export P4PORT=localhost:1666 P4USER=admin P4CLIENT=test-setup +mkdir -p /tmp/p4ws/main +cd /tmp/p4ws +p4 set P4PORT=localhost:1666 +p4 set P4USER=admin +p4 set P4CLIENT=test-setup +echo 'Hello from Perforce' > /tmp/p4ws/main/test.txt +p4 -p localhost:1666 -u admin -c test-setup add /tmp/p4ws/main/test.txt 2>/dev/null || true +p4 -p localhost:1666 -u admin -c test-setup submit -d "Initial commit" 2>/dev/null || true`}, + } + + for _, cmd := range commands { + _, _, err := container.Exec(ctx, cmd) + if err != nil { + log.Printf("Warning: p4d setup command failed: %v (cmd: %v)", err, cmd) + } + } + + return nil +} + +// TestPerforceVcsRootCRUD tests creating, reading, and deleting a Perforce VCS root. +func TestPerforceVcsRootCRUD(T *testing.T) { + if testEnvRef == nil || testEnvRef.network == nil { + T.Skip("test requires testcontainers with Docker network") + } + + ctx := context.Background() + p4Env, err := startP4D(ctx, testEnvRef.network.Name) + if err != nil { + T.Skipf("could not start p4d: %v", err) + } + defer p4Env.Cleanup() + + vcsRootID := "Sandbox_PerforceTest" + + T.Run("create perforce vcs root", func(t *testing.T) { + // Use the Docker network name for the P4 port so TeamCity server can reach it + p4Port := fmt.Sprintf("perforce-server:1666") + + root, err := client.CreateVcsRoot(api.CreateVcsRootRequest{ + ID: vcsRootID, + Name: "Perforce Test Depot", + VcsName: "perforce", + ProjectID: testProject, + Properties: api.NewPerforceVcsRootProperties( + p4Port, + "admin", + "", + "", + ), + }) + require.NoError(t, err) + assert.Equal(t, vcsRootID, root.ID) + assert.Equal(t, "perforce", root.VcsName) + t.Logf("Created VCS root: %s", root.ID) + }) + + T.Run("get perforce vcs root", func(t *testing.T) { + root, err := client.GetVcsRoot(vcsRootID) + require.NoError(t, err) + assert.Equal(t, "perforce", root.VcsName) + assert.Equal(t, "Perforce Test Depot", root.Name) + + // Verify properties + props := make(map[string]string) + for _, p := range root.Properties.Property { + props[p.Name] = p.Value + } + assert.Contains(t, props["port"], "perforce-server:1666") + assert.Equal(t, "admin", props["user"]) + t.Logf("VCS root properties: %v", props) + }) + + T.Run("list vcs roots includes perforce", func(t *testing.T) { + roots, err := client.GetVcsRoots(api.VcsRootOptions{Project: testProject}) + require.NoError(t, err) + + found := false + for _, r := range roots.VcsRoots { + if r.ID == vcsRootID { + found = true + assert.Equal(t, "perforce", r.VcsName) + break + } + } + assert.True(t, found, "should find perforce VCS root in list") + }) + + T.Run("vcs root exists", func(t *testing.T) { + assert.True(t, client.VcsRootExists(vcsRootID)) + assert.False(t, client.VcsRootExists("NonExistent_P4Root")) + }) + + T.Run("attach to build config", func(t *testing.T) { + // Create a new build config for this test + p4ConfigID := "Sandbox_PerforceDemo" + if !client.BuildTypeExists(p4ConfigID) { + _, err := client.CreateBuildType(testProject, api.CreateBuildTypeRequest{ + ID: p4ConfigID, + Name: "Perforce Demo", + }) + require.NoError(t, err) + } + + err := client.AttachVcsRoot(p4ConfigID, vcsRootID) + require.NoError(t, err) + t.Logf("Attached VCS root %s to build config %s", vcsRootID, p4ConfigID) + }) + + T.Run("delete perforce vcs root", func(t *testing.T) { + // First detach from build configs by deleting the build config + // (simpler than detaching VCS root entries) + p4ConfigID := "Sandbox_PerforceDemo" + if client.BuildTypeExists(p4ConfigID) { + // Delete the build config first to avoid "VCS root is in use" error + raw, err := client.RawRequest("DELETE", fmt.Sprintf("/app/rest/buildTypes/id:%s", p4ConfigID), nil, nil) + if err != nil { + t.Logf("Warning: could not delete build config: %v", err) + } else if raw.StatusCode >= 300 { + t.Logf("Warning: delete build config returned %d", raw.StatusCode) + } + } + + err := client.DeleteVcsRoot(vcsRootID) + require.NoError(t, err) + + assert.False(t, client.VcsRootExists(vcsRootID), "VCS root should be deleted") + }) +} + +// TestPerforceBuildWithVcsRoot tests running a build that uses a Perforce VCS root. +func TestPerforceBuildWithVcsRoot(T *testing.T) { + if testEnvRef == nil || testEnvRef.network == nil { + T.Skip("test requires testcontainers with Docker network") + } + + ctx := context.Background() + p4Env, err := startP4D(ctx, testEnvRef.network.Name) + if err != nil { + T.Skipf("could not start p4d: %v", err) + } + defer p4Env.Cleanup() + + // Create a Perforce VCS root + vcsRootID := "Sandbox_P4BuildTest" + p4ConfigID := "Sandbox_P4BuildDemo" + + // Cleanup from any previous run + if client.BuildTypeExists(p4ConfigID) { + client.RawRequest("DELETE", fmt.Sprintf("/app/rest/buildTypes/id:%s", p4ConfigID), nil, nil) + } + if client.VcsRootExists(vcsRootID) { + client.DeleteVcsRoot(vcsRootID) + } + + // Create VCS root pointing to the p4d container + root, err := client.CreateVcsRoot(api.CreateVcsRootRequest{ + ID: vcsRootID, + Name: "P4 Build Test", + VcsName: "perforce", + ProjectID: testProject, + Properties: api.NewPerforceVcsRootProperties( + "perforce-server:1666", + "admin", + "", + "", + ), + }) + require.NoError(T, err) + T.Logf("Created VCS root: %s", root.ID) + + defer func() { + if client.BuildTypeExists(p4ConfigID) { + client.RawRequest("DELETE", fmt.Sprintf("/app/rest/buildTypes/id:%s", p4ConfigID), nil, nil) + } + client.DeleteVcsRoot(vcsRootID) + }() + + // Create build config with a simple step + _, err = client.CreateBuildType(testProject, api.CreateBuildTypeRequest{ + ID: p4ConfigID, + Name: "P4 Build Demo", + }) + require.NoError(T, err) + + err = client.AttachVcsRoot(p4ConfigID, vcsRootID) + require.NoError(T, err) + + err = client.CreateBuildStep(p4ConfigID, api.BuildStep{ + Name: "Test P4", + Type: "simpleRunner", + Properties: api.PropertyList{ + Property: []api.Property{ + {Name: "script.content", Value: "echo 'Build from Perforce depot'\nls -la"}, + {Name: "use.custom.script", Value: "true"}, + }, + }, + }) + require.NoError(T, err) + + // Trigger a build + build, err := client.RunBuild(p4ConfigID, api.RunBuildOptions{ + Comment: "Perforce integration test", + }) + require.NoError(T, err) + T.Logf("Triggered build #%d", build.ID) + + // Wait for build to complete (it may fail if p4 checkout doesn't work, + // but the important thing is that the VCS root integration works) + buildID := fmt.Sprintf("%d", build.ID) + deadline := time.Now().Add(3 * time.Minute) + for time.Now().Before(deadline) { + build, err = client.GetBuild(buildID) + require.NoError(T, err) + if build.State == "finished" { + break + } + time.Sleep(5 * time.Second) + } + + T.Logf("Build finished: state=%s status=%s", build.State, build.Status) + + // Get build log to see what happened + buildLog, err := client.GetBuildLog(buildID) + if err == nil { + // Look for Perforce-related output in the log + if strings.Contains(buildLog, "Perforce") || strings.Contains(buildLog, "p4") { + T.Logf("Build log contains Perforce references") + } + // Log last 500 chars for debugging + if len(buildLog) > 500 { + T.Logf("Build log (tail):\n%s", buildLog[len(buildLog)-500:]) + } else { + T.Logf("Build log:\n%s", buildLog) + } + } + + // The build triggered successfully with a Perforce VCS root - that's the key assertion + assert.Equal(T, "finished", build.State, "build should have finished") +} + +// TestPerforceUploadDiffChanges tests uploading a Perforce-style diff as personal changes. +func TestPerforceUploadDiffChanges(T *testing.T) { + T.Parallel() + + // Perforce diffs in unified format should upload just like Git diffs + patch := []byte(`--- a/depot/main/test.txt ++++ b/depot/main/test.txt +@@ -1 +1 @@ +-Hello from Perforce ++Hello from Perforce - modified in personal build +`) + + changeID, err := client.UploadDiffChanges(patch, "Perforce personal build test") + require.NoError(T, err) + assert.NotEmpty(T, changeID) + T.Logf("Uploaded Perforce diff as change ID: %s", changeID) +} diff --git a/internal/api/vcsroots.go b/internal/api/vcsroots.go new file mode 100644 index 0000000..1b6de4f --- /dev/null +++ b/internal/api/vcsroots.go @@ -0,0 +1,167 @@ +package api + +import ( + "bytes" + "encoding/json" + "fmt" + "net/url" +) + +// VcsRoot represents a TeamCity VCS root +type VcsRoot struct { + ID string `json:"id,omitempty"` + Name string `json:"name,omitempty"` + VcsName string `json:"vcsName,omitempty"` // e.g. "jetbrains.git", "perforce" + ProjectID string `json:"projectId,omitempty"` + Href string `json:"href,omitempty"` + Properties PropertyList `json:"properties,omitempty"` + Project *Project `json:"project,omitempty"` +} + +// VcsRootList represents a list of VCS roots +type VcsRootList struct { + Count int `json:"count"` + VcsRoots []VcsRoot `json:"vcs-root"` +} + +// VcsRootOptions represents options for listing VCS roots +type VcsRootOptions struct { + Project string + Limit int +} + +// GetVcsRoots returns a list of VCS roots +func (c *Client) GetVcsRoots(opts VcsRootOptions) (*VcsRootList, error) { + locator := NewLocator(). + Add("project", opts.Project). + AddIntDefault("count", opts.Limit, 30) + + path := fmt.Sprintf("/app/rest/vcs-roots?locator=%s", locator.Encode()) + + var result VcsRootList + if err := c.get(path, &result); err != nil { + return nil, err + } + + return &result, nil +} + +// GetVcsRoot returns a single VCS root by ID +func (c *Client) GetVcsRoot(id string) (*VcsRoot, error) { + path := fmt.Sprintf("/app/rest/vcs-roots/id:%s", url.PathEscape(id)) + + var vcsRoot VcsRoot + if err := c.get(path, &vcsRoot); err != nil { + return nil, err + } + + return &vcsRoot, nil +} + +// CreateVcsRootRequest represents a request to create a VCS root +type CreateVcsRootRequest struct { + ID string `json:"id,omitempty"` + Name string `json:"name"` + VcsName string `json:"vcsName"` // "jetbrains.git", "perforce" + ProjectID string `json:"project,omitempty"` + Properties PropertyList `json:"properties,omitempty"` +} + +// createVcsRootPayload is the JSON format expected by the API +type createVcsRootPayload struct { + ID string `json:"id,omitempty"` + Name string `json:"name"` + VcsName string `json:"vcsName"` + Project *ProjectRef `json:"project,omitempty"` + Properties PropertyList `json:"properties,omitempty"` +} + +// ProjectRef is a reference to a project by ID +type ProjectRef struct { + ID string `json:"id"` +} + +// CreateVcsRoot creates a new VCS root +func (c *Client) CreateVcsRoot(req CreateVcsRootRequest) (*VcsRoot, error) { + payload := createVcsRootPayload{ + ID: req.ID, + Name: req.Name, + VcsName: req.VcsName, + Properties: req.Properties, + } + if req.ProjectID != "" { + payload.Project = &ProjectRef{ID: req.ProjectID} + } + + body, err := json.Marshal(payload) + if err != nil { + return nil, fmt.Errorf("failed to marshal request: %w", err) + } + + var vcsRoot VcsRoot + if err := c.post("/app/rest/vcs-roots", bytes.NewReader(body), &vcsRoot); err != nil { + return nil, err + } + + return &vcsRoot, nil +} + +// DeleteVcsRoot deletes a VCS root by ID +func (c *Client) DeleteVcsRoot(id string) error { + path := fmt.Sprintf("/app/rest/vcs-roots/id:%s", url.PathEscape(id)) + return c.doNoContent("DELETE", path, nil, "") +} + +// VcsRootExists checks if a VCS root exists +func (c *Client) VcsRootExists(id string) bool { + _, err := c.GetVcsRoot(id) + return err == nil +} + +// AttachVcsRoot attaches a VCS root to a build configuration +func (c *Client) AttachVcsRoot(buildTypeID string, vcsRootID string) error { + payload := struct { + ID string `json:"id"` + VcsRoot struct { + ID string `json:"id"` + } `json:"vcs-root"` + }{ + ID: vcsRootID, + } + payload.VcsRoot.ID = vcsRootID + + body, err := json.Marshal(payload) + if err != nil { + return fmt.Errorf("failed to marshal request: %w", err) + } + + path := fmt.Sprintf("/app/rest/buildTypes/id:%s/vcs-root-entries", buildTypeID) + return c.doNoContent("POST", path, bytes.NewReader(body), "") +} + +// Helper: creates a property list for a Perforce VCS root +func NewPerforceVcsRootProperties(port, user, password, stream string) PropertyList { + props := []Property{ + {Name: "port", Value: port}, + {Name: "user", Value: user}, + } + if password != "" { + props = append(props, Property{Name: "secure:passwd", Value: password}) + } + if stream != "" { + props = append(props, Property{Name: "stream", Value: stream}, + Property{Name: "use-client", Value: "false"}) + } + return PropertyList{Property: props} +} + +// Helper: creates a property list for a Git VCS root +func NewGitVcsRootProperties(url, branch string) PropertyList { + props := []Property{ + {Name: "url", Value: url}, + } + if branch != "" { + props = append(props, Property{Name: "branch", Value: branch}) + } + return PropertyList{Property: props} +} diff --git a/internal/api/vcsroots_test.go b/internal/api/vcsroots_test.go new file mode 100644 index 0000000..25e3665 --- /dev/null +++ b/internal/api/vcsroots_test.go @@ -0,0 +1,347 @@ +package api + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGetVcsRoots(T *testing.T) { + T.Parallel() + + T.Run("success", func(t *testing.T) { + t.Parallel() + + client := setupTestServer(t, func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "GET", r.Method) + assert.True(t, strings.HasPrefix(r.URL.Path, "/app/rest/vcs-roots")) + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(VcsRootList{ + Count: 2, + VcsRoots: []VcsRoot{ + {ID: "Project_GitRepo", Name: "Git Repo", VcsName: "jetbrains.git"}, + {ID: "Project_P4Depot", Name: "Perforce Depot", VcsName: "perforce"}, + }, + }) + }) + + roots, err := client.GetVcsRoots(VcsRootOptions{Project: "TestProject"}) + require.NoError(t, err) + assert.Equal(t, 2, roots.Count) + assert.Equal(t, "jetbrains.git", roots.VcsRoots[0].VcsName) + assert.Equal(t, "perforce", roots.VcsRoots[1].VcsName) + }) + + T.Run("empty", func(t *testing.T) { + t.Parallel() + + client := setupTestServer(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(VcsRootList{Count: 0, VcsRoots: nil}) + }) + + roots, err := client.GetVcsRoots(VcsRootOptions{}) + require.NoError(t, err) + assert.Equal(t, 0, roots.Count) + }) +} + +func TestGetVcsRoot(T *testing.T) { + T.Parallel() + + T.Run("git root", func(t *testing.T) { + t.Parallel() + + client := setupTestServer(t, func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "GET", r.Method) + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(VcsRoot{ + ID: "Project_GitRepo", + Name: "Git Repository", + VcsName: "jetbrains.git", + Properties: PropertyList{ + Property: []Property{ + {Name: "url", Value: "https://github.com/example/repo.git"}, + {Name: "branch", Value: "refs/heads/main"}, + }, + }, + }) + }) + + root, err := client.GetVcsRoot("Project_GitRepo") + require.NoError(t, err) + assert.Equal(t, "jetbrains.git", root.VcsName) + assert.Len(t, root.Properties.Property, 2) + }) + + T.Run("perforce root", func(t *testing.T) { + t.Parallel() + + client := setupTestServer(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(VcsRoot{ + ID: "Project_P4Depot", + Name: "Perforce Stream", + VcsName: "perforce", + Properties: PropertyList{ + Property: []Property{ + {Name: "port", Value: "ssl:perforce.example.com:1666"}, + {Name: "user", Value: "buildbot"}, + {Name: "stream", Value: "//depot/main"}, + }, + }, + }) + }) + + root, err := client.GetVcsRoot("Project_P4Depot") + require.NoError(t, err) + assert.Equal(t, "perforce", root.VcsName) + assert.Len(t, root.Properties.Property, 3) + }) + + T.Run("not found", func(t *testing.T) { + t.Parallel() + + client := setupTestServer(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + w.Write([]byte(`{"errors":[{"message":"VCS root not found"}]}`)) + }) + + _, err := client.GetVcsRoot("NonExistent") + assert.Error(t, err) + }) +} + +func TestCreateVcsRoot(T *testing.T) { + T.Parallel() + + T.Run("create git root", func(t *testing.T) { + t.Parallel() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "POST", r.Method) + assert.Equal(t, "/app/rest/vcs-roots", r.URL.Path) + + body, _ := io.ReadAll(r.Body) + var payload createVcsRootPayload + require.NoError(t, json.Unmarshal(body, &payload)) + + assert.Equal(t, "jetbrains.git", payload.VcsName) + assert.Equal(t, "My Git Repo", payload.Name) + assert.NotNil(t, payload.Project) + assert.Equal(t, "TestProject", payload.Project.ID) + + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(VcsRoot{ + ID: "TestProject_MyGitRepo", + Name: "My Git Repo", + VcsName: "jetbrains.git", + }) + })) + t.Cleanup(server.Close) + client := NewClient(server.URL, "test-token") + + root, err := client.CreateVcsRoot(CreateVcsRootRequest{ + Name: "My Git Repo", + VcsName: "jetbrains.git", + ProjectID: "TestProject", + Properties: NewGitVcsRootProperties( + "https://github.com/example/repo.git", + "refs/heads/main", + ), + }) + require.NoError(t, err) + assert.Equal(t, "TestProject_MyGitRepo", root.ID) + }) + + T.Run("create perforce root", func(t *testing.T) { + t.Parallel() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "POST", r.Method) + + body, _ := io.ReadAll(r.Body) + var payload createVcsRootPayload + require.NoError(t, json.Unmarshal(body, &payload)) + + assert.Equal(t, "perforce", payload.VcsName) + assert.Equal(t, "P4 Stream", payload.Name) + + // Verify Perforce-specific properties + props := make(map[string]string) + for _, p := range payload.Properties.Property { + props[p.Name] = p.Value + } + assert.Equal(t, "ssl:p4.example.com:1666", props["port"]) + assert.Equal(t, "buildbot", props["user"]) + assert.Equal(t, "//depot/main", props["stream"]) + + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(VcsRoot{ + ID: "TestProject_P4Stream", + Name: "P4 Stream", + VcsName: "perforce", + }) + })) + t.Cleanup(server.Close) + client := NewClient(server.URL, "test-token") + + root, err := client.CreateVcsRoot(CreateVcsRootRequest{ + Name: "P4 Stream", + VcsName: "perforce", + ProjectID: "TestProject", + Properties: NewPerforceVcsRootProperties( + "ssl:p4.example.com:1666", + "buildbot", + "secret123", + "//depot/main", + ), + }) + require.NoError(t, err) + assert.Equal(t, "perforce", root.VcsName) + }) +} + +func TestDeleteVcsRoot(T *testing.T) { + T.Parallel() + + T.Run("success", func(t *testing.T) { + t.Parallel() + + client := setupTestServer(t, func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "DELETE", r.Method) + assert.True(t, strings.Contains(r.URL.Path, "/vcs-roots/id:")) + w.WriteHeader(http.StatusNoContent) + }) + + err := client.DeleteVcsRoot("TestProject_GitRepo") + require.NoError(t, err) + }) + + T.Run("not found", func(t *testing.T) { + t.Parallel() + + client := setupTestServer(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + w.Write([]byte(`{"errors":[{"message":"VCS root not found"}]}`)) + }) + + err := client.DeleteVcsRoot("NonExistent") + assert.Error(t, err) + }) +} + +func TestVcsRootExists(T *testing.T) { + T.Parallel() + + T.Run("exists", func(t *testing.T) { + t.Parallel() + + client := setupTestServer(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(VcsRoot{ID: "TestVcsRoot", Name: "Test"}) + }) + + assert.True(t, client.VcsRootExists("TestVcsRoot")) + }) + + T.Run("not exists", func(t *testing.T) { + t.Parallel() + + client := setupTestServer(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + }) + + assert.False(t, client.VcsRootExists("NonExistent")) + }) +} + +func TestAttachVcsRoot(T *testing.T) { + T.Parallel() + + T.Run("success", func(t *testing.T) { + t.Parallel() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "POST", r.Method) + assert.True(t, strings.HasSuffix(r.URL.Path, "/vcs-root-entries")) + + body, _ := io.ReadAll(r.Body) + assert.Contains(t, string(body), "TestProject_P4Root") + + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(server.Close) + client := NewClient(server.URL, "test-token") + + err := client.AttachVcsRoot("Sandbox_Build", "TestProject_P4Root") + require.NoError(t, err) + }) +} + +func TestNewPerforceVcsRootProperties(T *testing.T) { + T.Parallel() + + T.Run("with stream", func(t *testing.T) { + t.Parallel() + + props := NewPerforceVcsRootProperties("p4.example.com:1666", "builder", "pass", "//depot/main") + + names := make(map[string]string) + for _, p := range props.Property { + names[p.Name] = p.Value + } + assert.Equal(t, "p4.example.com:1666", names["port"]) + assert.Equal(t, "builder", names["user"]) + assert.Equal(t, "pass", names["secure:passwd"]) + assert.Equal(t, "//depot/main", names["stream"]) + assert.Equal(t, "false", names["use-client"]) + }) + + T.Run("without password", func(t *testing.T) { + t.Parallel() + + props := NewPerforceVcsRootProperties("p4:1666", "user", "", "//depot/dev") + + names := make(map[string]string) + for _, p := range props.Property { + names[p.Name] = p.Value + } + assert.Equal(t, "p4:1666", names["port"]) + _, hasPassword := names["secure:passwd"] + assert.False(t, hasPassword) + }) +} + +func TestNewGitVcsRootProperties(T *testing.T) { + T.Parallel() + + T.Run("with branch", func(t *testing.T) { + t.Parallel() + + props := NewGitVcsRootProperties("https://github.com/example/repo.git", "refs/heads/main") + + names := make(map[string]string) + for _, p := range props.Property { + names[p.Name] = p.Value + } + assert.Equal(t, "https://github.com/example/repo.git", names["url"]) + assert.Equal(t, "refs/heads/main", names["branch"]) + }) + + T.Run("without branch", func(t *testing.T) { + t.Parallel() + + props := NewGitVcsRootProperties("https://github.com/example/repo.git", "") + + assert.Len(t, props.Property, 1) + assert.Equal(t, "url", props.Property[0].Name) + }) +} diff --git a/internal/cmd/run_analysis.go b/internal/cmd/run_analysis.go index 5a2c1d8..a727390 100644 --- a/internal/cmd/run_analysis.go +++ b/internal/cmd/run_analysis.go @@ -60,17 +60,20 @@ func runRunChanges(runID string, opts *runChangesOptions) error { fmt.Printf("CHANGES (%d %s)\n\n", changes.Count, english.PluralWord(changes.Count, "commit", "commits")) - var firstSHA, lastSHA string + // Detect VCS provider for formatting (falls back to Git-style if unavailable) + vcs := DetectVCS() + if vcs == nil { + vcs = &GitProvider{} // default to Git formatting + } + + var firstRev, lastRev string for i, c := range changes.Change { if i == 0 { - lastSHA = c.Version + lastRev = c.Version } - firstSHA = c.Version + firstRev = c.Version - sha := c.Version - if len(sha) > 7 { - sha = sha[:7] - } + rev := vcs.FormatRevision(c.Version) date := "" if c.Date != "" { @@ -79,7 +82,7 @@ func runRunChanges(runID string, opts *runChangesOptions) error { } } - fmt.Printf("%s %s %s\n", output.Yellow(sha), output.Faint(c.Username), output.Faint(date)) + fmt.Printf("%s %s %s\n", output.Yellow(rev), output.Faint(c.Username), output.Faint(date)) comment := strings.TrimSpace(c.Comment) if idx := strings.Index(comment, "\n"); idx > 0 { @@ -104,16 +107,8 @@ func runRunChanges(runID string, opts *runChangesOptions) error { fmt.Println() } - if firstSHA != "" && lastSHA != "" && firstSHA != lastSHA { - first := firstSHA - last := lastSHA - if len(first) > 7 { - first = first[:7] - } - if len(last) > 7 { - last = last[:7] - } - fmt.Printf("%s git diff %s^..%s\n", output.Faint("# For full diff:"), first, last) + if firstRev != "" && lastRev != "" && firstRev != lastRev { + fmt.Printf("%s %s\n", output.Faint("# For full diff:"), vcs.DiffHint(firstRev, lastRev)) } return nil diff --git a/internal/cmd/run_lifecycle.go b/internal/cmd/run_lifecycle.go index 2e1278a..5c24b5c 100644 --- a/internal/cmd/run_lifecycle.go +++ b/internal/cmd/run_lifecycle.go @@ -70,7 +70,7 @@ func newRunStartCmd() *cobra.Command { cmd.Flags().StringVarP(&opts.comment, "comment", "m", "", "Run comment") cmd.Flags().StringSliceVarP(&opts.tags, "tag", "t", nil, "Run tags (can be repeated)") cmd.Flags().BoolVar(&opts.personal, "personal", false, "Run as personal build") - localChangesFlag := cmd.Flags().VarPF(&localChangesValue{val: &opts.localChanges}, "local-changes", "l", "Include local changes (git, -, or path; default: git)") + localChangesFlag := cmd.Flags().VarPF(&localChangesValue{val: &opts.localChanges}, "local-changes", "l", "Include local changes (git, p4, auto, -, or path; default: git)") localChangesFlag.NoOptDefVal = "git" cmd.Flags().BoolVar(&opts.noPush, "no-push", false, "Skip auto-push of branch to remote") cmd.Flags().BoolVar(&opts.cleanSources, "clean", false, "Clean sources before run") @@ -139,24 +139,26 @@ func runRunStart(jobID string, opts *runStartOptions) error { var headCommit string if opts.localChanges != "" && opts.branch == "" { - if !isGitRepo() { + vcs := DetectVCS() + if vcs == nil { return tcerrors.WithSuggestion( - "not a git repository", - "Run this command from within a git repository, or specify --branch explicitly", + "no supported VCS detected", + "Run this command from within a git repository or Perforce workspace, or specify --branch explicitly", ) } - branch, err := getCurrentBranch() + branch, err := vcs.GetCurrentBranch() if err != nil { return err } opts.branch = branch - output.Info("Using current branch: %s", branch) + output.Info("Using current %s branch: %s", vcs.Name(), branch) } if opts.localChanges != "" && !opts.noPush { - if !branchExistsOnRemote(opts.branch) { + vcs := DetectVCS() + if vcs != nil && !vcs.BranchExistsOnRemote(opts.branch) { output.Info("Pushing branch to remote...") - if err := pushBranch(opts.branch); err != nil { + if err := vcs.PushBranch(opts.branch); err != nil { return err } output.Success("Branch pushed to remote") @@ -164,11 +166,14 @@ func runRunStart(jobID string, opts *runStartOptions) error { } if opts.localChanges != "" { - commit, err := getHeadCommit() - if err != nil { - return err + vcs := DetectVCS() + if vcs != nil { + commit, err := vcs.GetHeadRevision() + if err != nil { + return err + } + headCommit = commit } - headCommit = commit } client, err := getClient() @@ -199,7 +204,7 @@ func runRunStart(jobID string, opts *runStartOptions) error { opts.personal = true } - build, err := client.RunBuild(jobID, api.RunBuildOptions{ + buildOpts := api.RunBuildOptions{ Branch: opts.branch, Params: opts.params, SystemProps: opts.systemProps, @@ -214,7 +219,14 @@ func runRunStart(jobID string, opts *runStartOptions) error { Tags: opts.tags, PersonalChangeID: personalChangeID, Revision: headCommit, - }) + } + + // Wire up VCS-specific branch formatting when a provider is available + if vcs := DetectVCS(); vcs != nil { + buildOpts.VCSBranchFormatter = vcs.FormatVCSBranch + } + + build, err := client.RunBuild(jobID, buildOpts) if err != nil { return err } @@ -536,6 +548,44 @@ func loadLocalChanges(source string) ([]byte, error) { ) } return patch, nil + case "p4", "perforce": + p4 := &PerforceProvider{} + if !p4.IsAvailable() { + return nil, tcerrors.WithSuggestion( + "not a Perforce workspace", + "Run this command from within a Perforce workspace, or use --local-changes to specify a diff file", + ) + } + patch, err := p4.GetLocalDiff() + if err != nil { + return nil, err + } + if len(patch) == 0 { + return nil, tcerrors.WithSuggestion( + "no pending changes found", + "Open files for edit before running a personal build, or use --local-changes to specify a diff file", + ) + } + return patch, nil + case "auto", "": + vcs := DetectVCS() + if vcs == nil { + return nil, tcerrors.WithSuggestion( + "no supported VCS detected", + "Run this command from within a git repository or Perforce workspace, or use --local-changes to specify a diff file", + ) + } + patch, err := vcs.GetLocalDiff() + if err != nil { + return nil, err + } + if len(patch) == 0 { + return nil, tcerrors.WithSuggestion( + "no uncommitted changes found", + "Make some changes to your files before running a personal build, or use --local-changes to specify a diff file", + ) + } + return patch, nil case "-": patch, err := io.ReadAll(os.Stdin) if err != nil { diff --git a/internal/cmd/testutil_test.go b/internal/cmd/testutil_test.go index 585a362..19f38f8 100644 --- a/internal/cmd/testutil_test.go +++ b/internal/cmd/testutil_test.go @@ -582,6 +582,37 @@ func setupMockClient(t *testing.T) *TestServer { w.WriteHeader(http.StatusNoContent) }) + // VCS Roots + ts.Handle("GET /app/rest/vcs-roots", func(w http.ResponseWriter, r *http.Request) { + JSON(w, api.VcsRootList{ + Count: 2, + VcsRoots: []api.VcsRoot{ + {ID: "TestProject_GitRepo", Name: "Git Repository", VcsName: "jetbrains.git"}, + {ID: "TestProject_P4Depot", Name: "Perforce Depot", VcsName: "perforce"}, + }, + }) + }) + + ts.Handle("GET /app/rest/vcs-roots/id:", func(w http.ResponseWriter, r *http.Request) { + JSON(w, api.VcsRoot{ + ID: "TestProject_GitRepo", + Name: "Git Repository", + VcsName: "jetbrains.git", + }) + }) + + ts.Handle("POST /app/rest/vcs-roots", func(w http.ResponseWriter, r *http.Request) { + JSON(w, api.VcsRoot{ + ID: "TestProject_NewRoot", + Name: "New Root", + VcsName: "perforce", + }) + }) + + ts.Handle("DELETE /app/rest/vcs-roots/id:", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + // Versioned Settings ts.Handle("GET /app/rest/projects/TestProject/versionedSettings/config", func(w http.ResponseWriter, r *http.Request) { JSON(w, api.VersionedSettingsConfig{ diff --git a/internal/cmd/vcs_git.go b/internal/cmd/vcs_git.go new file mode 100644 index 0000000..1dca55e --- /dev/null +++ b/internal/cmd/vcs_git.go @@ -0,0 +1,63 @@ +package cmd + +import ( + "fmt" + "strings" +) + +// GitProvider implements VCSProvider for Git repositories. +type GitProvider struct{} + +func (g *GitProvider) Name() string { + return "git" +} + +func (g *GitProvider) IsAvailable() bool { + return isGitRepo() +} + +func (g *GitProvider) GetCurrentBranch() (string, error) { + return getCurrentBranch() +} + +func (g *GitProvider) GetHeadRevision() (string, error) { + return getHeadCommit() +} + +func (g *GitProvider) GetLocalDiff() ([]byte, error) { + return getGitDiff() +} + +func (g *GitProvider) BranchExistsOnRemote(branch string) bool { + return branchExistsOnRemote(branch) +} + +func (g *GitProvider) PushBranch(branch string) error { + return pushBranch(branch) +} + +func (g *GitProvider) FormatRevision(rev string) string { + if len(rev) > 7 { + return rev[:7] + } + return rev +} + +func (g *GitProvider) FormatVCSBranch(branch string) string { + if branch != "" && !strings.HasPrefix(branch, "refs/") { + return "refs/heads/" + branch + } + return branch +} + +func (g *GitProvider) DiffHint(firstRev, lastRev string) string { + first := firstRev + last := lastRev + if len(first) > 7 { + first = first[:7] + } + if len(last) > 7 { + last = last[:7] + } + return fmt.Sprintf("git diff %s^..%s", first, last) +} diff --git a/internal/cmd/vcs_perforce.go b/internal/cmd/vcs_perforce.go new file mode 100644 index 0000000..17afc24 --- /dev/null +++ b/internal/cmd/vcs_perforce.go @@ -0,0 +1,238 @@ +package cmd + +import ( + "fmt" + "os/exec" + "strings" + + tcerrors "github.com/JetBrains/teamcity-cli/internal/errors" +) + +// PerforceProvider implements VCSProvider for Perforce (Helix Core) workspaces. +type PerforceProvider struct{} + +func (p *PerforceProvider) Name() string { + return "perforce" +} + +func (p *PerforceProvider) IsAvailable() bool { + return isPerforceWorkspace() +} + +func (p *PerforceProvider) GetCurrentBranch() (string, error) { + return getPerforceStream() +} + +func (p *PerforceProvider) GetHeadRevision() (string, error) { + return getPerforceChangelist() +} + +func (p *PerforceProvider) GetLocalDiff() ([]byte, error) { + return getPerforceDiff() +} + +func (p *PerforceProvider) BranchExistsOnRemote(branch string) bool { + // Perforce streams/depot paths always exist on the server if the workspace is valid. + // Check by querying the stream spec. + if strings.HasPrefix(branch, "//") { + cmd := exec.Command("p4", "stream", "-o", branch) + err := cmd.Run() + return err == nil + } + return true +} + +func (p *PerforceProvider) PushBranch(_ string) error { + // Perforce doesn't have a push concept; changes are submitted directly. + return nil +} + +func (p *PerforceProvider) FormatRevision(rev string) string { + // Perforce changelist numbers are typically short; return as-is. + return rev +} + +func (p *PerforceProvider) FormatVCSBranch(branch string) string { + // Perforce depot paths are used as-is in TeamCity VCS branch references. + return branch +} + +func (p *PerforceProvider) DiffHint(firstRev, lastRev string) string { + return fmt.Sprintf("p4 changes -l @%s,@%s", firstRev, lastRev) +} + +// isPerforceWorkspace checks if the current directory is inside a Perforce workspace. +func isPerforceWorkspace() bool { + cmd := exec.Command("p4", "info") + out, err := cmd.Output() + if err != nil { + return false + } + // p4 info returns "Client unknown" when not in a valid workspace + outStr := string(out) + if strings.Contains(outStr, "Client unknown") { + return false + } + // Verify there's a Client root set + return strings.Contains(outStr, "Client root:") +} + +// getPerforceStream returns the current Perforce stream depot path. +// Falls back to the depot path if streams are not used. +func getPerforceStream() (string, error) { + // Try to get the stream from the current client spec + clientName, err := getPerforceClientName() + if err != nil { + return "", err + } + + cmd := exec.Command("p4", "-ztag", "client", "-o", clientName) + out, err := cmd.Output() + if err != nil { + return "", tcerrors.WithSuggestion( + "failed to get Perforce client spec", + "Ensure p4 is configured and you are in a valid workspace", + ) + } + + // Parse the stream field from ztag output + for _, line := range strings.Split(string(out), "\n") { + line = strings.TrimSpace(line) + if strings.HasPrefix(line, "... Stream ") { + stream := strings.TrimPrefix(line, "... Stream ") + return strings.TrimSpace(stream), nil + } + } + + // If no stream, try to extract the depot path from the client mapping + for _, line := range strings.Split(string(out), "\n") { + line = strings.TrimSpace(line) + if strings.HasPrefix(line, "... View0 ") { + view := strings.TrimPrefix(line, "... View0 ") + // View format: "//depot/path/... //client/..." + parts := strings.Fields(view) + if len(parts) >= 1 { + depotPath := strings.TrimSuffix(parts[0], "/...") + return depotPath, nil + } + } + } + + return "", tcerrors.WithSuggestion( + "could not determine Perforce stream or depot path", + "Ensure your workspace is mapped to a stream or depot path", + ) +} + +// getPerforceClientName returns the name of the current Perforce client/workspace. +func getPerforceClientName() (string, error) { + cmd := exec.Command("p4", "-ztag", "info") + out, err := cmd.Output() + if err != nil { + return "", tcerrors.WithSuggestion( + "failed to get Perforce info", + "Ensure p4 is installed and P4PORT/P4USER/P4CLIENT are configured", + ) + } + + for _, line := range strings.Split(string(out), "\n") { + line = strings.TrimSpace(line) + if strings.HasPrefix(line, "... clientName ") { + return strings.TrimPrefix(line, "... clientName "), nil + } + } + + return "", tcerrors.WithSuggestion( + "no Perforce client found", + "Set P4CLIENT or run 'p4 set P4CLIENT='", + ) +} + +// getPerforceChangelist returns the latest submitted changelist number +// that affects the current workspace. +func getPerforceChangelist() (string, error) { + clientName, err := getPerforceClientName() + if err != nil { + return "", err + } + + // Get the latest changelist synced to this workspace + cmd := exec.Command("p4", "changes", "-m1", "-s", "submitted", "@"+clientName) + out, err := cmd.Output() + if err != nil { + return "", tcerrors.WithSuggestion( + "failed to get Perforce changelist", + "Ensure you have submitted changes in your workspace", + ) + } + + // Output format: "Change 12345 on 2024/01/01 by user@client 'description'" + outStr := strings.TrimSpace(string(out)) + if outStr == "" { + return "", tcerrors.WithSuggestion( + "no submitted changelists found", + "Submit at least one changelist, or specify --branch explicitly", + ) + } + + fields := strings.Fields(outStr) + if len(fields) >= 2 && fields[0] == "Change" { + return fields[1], nil + } + + return "", tcerrors.WithSuggestion( + "unexpected p4 changes output", + "Ensure p4 is working correctly", + ) +} + +// getPerforceDiff generates a unified diff from pending changes in the default changelist. +func getPerforceDiff() ([]byte, error) { + // First check if there are any open files + openCmd := exec.Command("p4", "opened") + openOut, err := openCmd.Output() + if err != nil { + return nil, tcerrors.WithSuggestion( + "failed to list opened files", + "Ensure you are in a Perforce workspace with pending changes", + ) + } + + if strings.TrimSpace(string(openOut)) == "" { + return nil, nil // No pending changes + } + + // Generate unified diff of all opened files + cmd := exec.Command("p4", "diff", "-du") + out, err := cmd.Output() + if err != nil { + // p4 diff returns exit code 1 if there are differences (which is expected) + if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { + return out, nil + } + return nil, tcerrors.WithSuggestion( + "failed to generate Perforce diff", + "Ensure you have pending changes in your workspace", + ) + } + + return out, nil +} + +// getPerforcePort returns the Perforce server address (P4PORT). +func getPerforcePort() (string, error) { + cmd := exec.Command("p4", "-ztag", "info") + out, err := cmd.Output() + if err != nil { + return "", err + } + + for _, line := range strings.Split(string(out), "\n") { + line = strings.TrimSpace(line) + if strings.HasPrefix(line, "... serverAddress ") { + return strings.TrimPrefix(line, "... serverAddress "), nil + } + } + + return "", fmt.Errorf("P4PORT not found") +} diff --git a/internal/cmd/vcs_provider.go b/internal/cmd/vcs_provider.go new file mode 100644 index 0000000..96ee3b9 --- /dev/null +++ b/internal/cmd/vcs_provider.go @@ -0,0 +1,71 @@ +package cmd + +// VCSProvider abstracts version control operations used by the CLI. +// Implementations exist for Git and Perforce. +type VCSProvider interface { + // Name returns the VCS type name (e.g., "git", "perforce"). + Name() string + + // IsAvailable returns true if this VCS is detected in the current working directory. + IsAvailable() bool + + // GetCurrentBranch returns the current branch or stream name. + // For Git, this is the branch name. For Perforce, this is the stream depot path. + GetCurrentBranch() (string, error) + + // GetHeadRevision returns the current revision identifier. + // For Git, this is a commit SHA. For Perforce, this is a changelist number. + GetHeadRevision() (string, error) + + // GetLocalDiff returns a unified diff of uncommitted local changes. + // For Git, this is `git diff HEAD` including untracked files. + // For Perforce, this is `p4 diff -du` of pending changelists. + GetLocalDiff() ([]byte, error) + + // BranchExistsOnRemote checks whether the branch/stream exists on the remote server. + BranchExistsOnRemote(branch string) bool + + // PushBranch pushes the given branch to the remote. + // For Perforce, this is a no-op (changes are submitted directly). + PushBranch(branch string) error + + // FormatRevision returns a short display form of a revision identifier. + // For Git, truncates SHA to 7 chars. For Perforce, returns the changelist number as-is. + FormatRevision(rev string) string + + // FormatVCSBranch converts a user-facing branch name to the VCS-specific ref format + // used in TeamCity API calls. + // For Git: "main" -> "refs/heads/main". For Perforce: returns depot path as-is. + FormatVCSBranch(branch string) string + + // DiffHint returns a human-readable hint for viewing the full diff between two revisions. + // For Git: "git diff abc1234^..def5678". For Perforce: "p4 changes -l @123,@456". + DiffHint(firstRev, lastRev string) string +} + +// DetectVCS returns the VCS provider for the current working directory, +// or nil if no supported VCS is detected. +func DetectVCS() VCSProvider { + providers := []VCSProvider{ + &GitProvider{}, + &PerforceProvider{}, + } + for _, p := range providers { + if p.IsAvailable() { + return p + } + } + return nil +} + +// DetectVCSByName returns the VCS provider matching the given name, or nil. +func DetectVCSByName(name string) VCSProvider { + switch name { + case "git": + return &GitProvider{} + case "p4", "perforce": + return &PerforceProvider{} + default: + return nil + } +} diff --git a/internal/cmd/vcs_provider_test.go b/internal/cmd/vcs_provider_test.go new file mode 100644 index 0000000..5d5f31c --- /dev/null +++ b/internal/cmd/vcs_provider_test.go @@ -0,0 +1,198 @@ +package cmd + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestGitProvider_Name(t *testing.T) { + p := &GitProvider{} + assert.Equal(t, "git", p.Name()) +} + +func TestGitProvider_FormatRevision(t *testing.T) { + p := &GitProvider{} + + // Long SHA gets truncated to 7 + assert.Equal(t, "abc1234", p.FormatRevision("abc12345678901234567890123456789012345678")) + + // Short string stays the same + assert.Equal(t, "abc", p.FormatRevision("abc")) + + // Exactly 7 chars stays the same + assert.Equal(t, "abc1234", p.FormatRevision("abc1234")) +} + +func TestGitProvider_FormatVCSBranch(t *testing.T) { + p := &GitProvider{} + + assert.Equal(t, "refs/heads/main", p.FormatVCSBranch("main")) + assert.Equal(t, "refs/heads/feature/test", p.FormatVCSBranch("feature/test")) + assert.Equal(t, "refs/tags/v1.0", p.FormatVCSBranch("refs/tags/v1.0")) + assert.Equal(t, "", p.FormatVCSBranch("")) +} + +func TestGitProvider_DiffHint(t *testing.T) { + p := &GitProvider{} + + hint := p.DiffHint("abc1234567890", "def1234567890") + assert.Equal(t, "git diff abc1234^..def1234", hint) +} + +func TestGitProvider_IsAvailable(t *testing.T) { + t.Run("in git repo", func(t *testing.T) { + dir := setupGitRepo(t) + restore := chdir(t, dir) + defer restore() + + p := &GitProvider{} + assert.True(t, p.IsAvailable()) + }) + + t.Run("not in git repo", func(t *testing.T) { + dir := t.TempDir() + restore := chdir(t, dir) + defer restore() + + p := &GitProvider{} + assert.False(t, p.IsAvailable()) + }) +} + +func TestGitProvider_GetCurrentBranch(t *testing.T) { + dir := setupGitRepo(t) + restore := chdir(t, dir) + defer restore() + + createFile(t, dir, "test.txt", "content") + runGit(t, dir, "add", ".") + runGit(t, dir, "commit", "-m", "initial") + + p := &GitProvider{} + branch, err := p.GetCurrentBranch() + assert.NoError(t, err) + assert.Contains(t, []string{"main", "master"}, branch) +} + +func TestGitProvider_GetHeadRevision(t *testing.T) { + dir := setupGitRepo(t) + restore := chdir(t, dir) + defer restore() + + createFile(t, dir, "test.txt", "content") + runGit(t, dir, "add", ".") + runGit(t, dir, "commit", "-m", "initial") + + p := &GitProvider{} + rev, err := p.GetHeadRevision() + assert.NoError(t, err) + assert.Regexp(t, "^[0-9a-f]{40}$", rev) +} + +func TestGitProvider_GetLocalDiff(t *testing.T) { + dir := setupGitRepo(t) + restore := chdir(t, dir) + defer restore() + + createFile(t, dir, "test.txt", "content") + runGit(t, dir, "add", ".") + runGit(t, dir, "commit", "-m", "initial") + + // Make changes + createFile(t, dir, "test.txt", "modified") + + p := &GitProvider{} + diff, err := p.GetLocalDiff() + assert.NoError(t, err) + assert.Contains(t, string(diff), "modified") +} + +func TestPerforceProvider_Name(t *testing.T) { + p := &PerforceProvider{} + assert.Equal(t, "perforce", p.Name()) +} + +func TestPerforceProvider_FormatRevision(t *testing.T) { + p := &PerforceProvider{} + + // Perforce changelist numbers are returned as-is + assert.Equal(t, "12345", p.FormatRevision("12345")) + assert.Equal(t, "1", p.FormatRevision("1")) +} + +func TestPerforceProvider_FormatVCSBranch(t *testing.T) { + p := &PerforceProvider{} + + // Perforce depot paths pass through unchanged + assert.Equal(t, "//depot/main", p.FormatVCSBranch("//depot/main")) + assert.Equal(t, "//stream/dev", p.FormatVCSBranch("//stream/dev")) +} + +func TestPerforceProvider_DiffHint(t *testing.T) { + p := &PerforceProvider{} + + hint := p.DiffHint("100", "200") + assert.Equal(t, "p4 changes -l @100,@200", hint) +} + +func TestPerforceProvider_PushBranch(t *testing.T) { + // Push is a no-op for Perforce + p := &PerforceProvider{} + err := p.PushBranch("//depot/main") + assert.NoError(t, err) +} + +func TestPerforceProvider_IsAvailable(t *testing.T) { + // Without Perforce installed, should return false + dir := t.TempDir() + restore := chdir(t, dir) + defer restore() + + p := &PerforceProvider{} + // In CI without p4 binary, this should be false + // We don't assert true/false since it depends on the environment + _ = p.IsAvailable() +} + +func TestDetectVCS_Git(t *testing.T) { + dir := setupGitRepo(t) + restore := chdir(t, dir) + defer restore() + + vcs := DetectVCS() + assert.NotNil(t, vcs) + assert.Equal(t, "git", vcs.Name()) +} + +func TestDetectVCS_NoVCS(t *testing.T) { + dir := t.TempDir() + restore := chdir(t, dir) + defer restore() + + vcs := DetectVCS() + assert.Nil(t, vcs) +} + +func TestDetectVCSByName(t *testing.T) { + tests := []struct { + name string + expected string + }{ + {"git", "git"}, + {"p4", "perforce"}, + {"perforce", "perforce"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + vcs := DetectVCSByName(tc.name) + assert.NotNil(t, vcs) + assert.Equal(t, tc.expected, vcs.Name()) + }) + } + + t.Run("unknown", func(t *testing.T) { + vcs := DetectVCSByName("svn") + assert.Nil(t, vcs) + }) +} From cf0b7881c686577f00f3f28d52ca34b94e1c5a86 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 7 Feb 2026 10:37:52 +0000 Subject: [PATCH 2/4] refactor: Clean up Perforce VCS support based on code review - Consolidate 4 DetectVCS() calls in runRunStart into 1 - Unify loadLocalChanges through VCS provider (git/p4/auto all use same path) - Remove VCSBranchFormatter closure from API struct (heuristic handles both VCS types) - Add 10s timeout to all p4 subprocess calls via exec.CommandContext - Extract p4Output/p4Run/p4ZtagField helpers to DRY up ztag parsing - Remove dead code: getPerforcePort(), unused perforceTestEnv fields - Replace time.Sleep with readiness polling in p4d integration test - Fix populateP4Depot to propagate errors instead of swallowing - Fix url parameter shadow in NewGitVcsRootProperties https://claude.ai/code/session_01Buy3ZEvVTWwhkQHU4H442q --- internal/api/builds.go | 8 +- internal/api/perforce_test.go | 31 ++++- internal/api/vcsroots.go | 4 +- internal/cmd/git_helpers_test.go | 4 +- internal/cmd/run_lifecycle.go | 147 ++++++++------------ internal/cmd/vcs_perforce.go | 227 +++++++++++-------------------- 6 files changed, 168 insertions(+), 253 deletions(-) diff --git a/internal/api/builds.go b/internal/api/builds.go index f7cb297..a9219e1 100644 --- a/internal/api/builds.go +++ b/internal/api/builds.go @@ -120,8 +120,7 @@ type RunBuildOptions struct { AgentID int Tags []string PersonalChangeID string - Revision string // Base revision (commit SHA or changelist number) for personal builds - VCSBranchFormatter func(string) string `json:"-"` // Optional: converts branch name to VCS-specific ref format + Revision string // Base revision (commit SHA or changelist number) for personal builds } // RunBuild runs a new build with full options @@ -185,10 +184,7 @@ func (c *Client) RunBuild(buildTypeID string, opts RunBuildOptions) (*Build, err if opts.Revision != "" { vcsBranch := opts.Branch - if opts.VCSBranchFormatter != nil { - vcsBranch = opts.VCSBranchFormatter(vcsBranch) - } else if vcsBranch != "" && !strings.HasPrefix(vcsBranch, "refs/") && !strings.HasPrefix(vcsBranch, "//") { - // Default: assume Git-style refs unless it looks like a Perforce depot path + if vcsBranch != "" && !strings.HasPrefix(vcsBranch, "refs/") && !strings.HasPrefix(vcsBranch, "//") { vcsBranch = "refs/heads/" + vcsBranch } req.Revisions = &Revisions{ diff --git a/internal/api/perforce_test.go b/internal/api/perforce_test.go index bfe4819..6e4dadf 100644 --- a/internal/api/perforce_test.go +++ b/internal/api/perforce_test.go @@ -28,8 +28,6 @@ type perforceTestEnv struct { port string host string ctx context.Context - vcsRootID string - configID string } func (e *perforceTestEnv) Cleanup() { @@ -77,10 +75,11 @@ func startP4D(ctx context.Context, networkName string) (*perforceTestEnv, error) log.Printf("P4D running at %s:%s", host, env.port) - // Give p4d a moment to fully initialize - time.Sleep(3 * time.Second) + if err := waitForP4D(ctx, container); err != nil { + env.Cleanup() + return nil, fmt.Errorf("p4d not ready: %w", err) + } - // Populate with test depot content if err := populateP4Depot(ctx, container); err != nil { env.Cleanup() return nil, fmt.Errorf("populate depot: %w", err) @@ -89,6 +88,25 @@ func startP4D(ctx context.Context, networkName string) (*perforceTestEnv, error) return env, nil } +// waitForP4D polls p4d until it accepts connections. +func waitForP4D(ctx context.Context, container testcontainers.Container) error { + deadline := time.After(30 * time.Second) + for { + select { + case <-deadline: + return fmt.Errorf("timeout waiting for p4d to accept connections") + case <-ctx.Done(): + return ctx.Err() + default: + _, _, err := container.Exec(ctx, []string{"p4", "-p", "localhost:1666", "info"}) + if err == nil { + return nil + } + time.Sleep(500 * time.Millisecond) + } + } +} + // populateP4Depot creates a test depot with files in the p4d container. func populateP4Depot(ctx context.Context, container testcontainers.Container) error { commands := [][]string{ @@ -111,10 +129,9 @@ p4 -p localhost:1666 -u admin -c test-setup submit -d "Initial commit" 2>/dev/nu for _, cmd := range commands { _, _, err := container.Exec(ctx, cmd) if err != nil { - log.Printf("Warning: p4d setup command failed: %v (cmd: %v)", err, cmd) + return fmt.Errorf("p4d setup failed: %w", err) } } - return nil } diff --git a/internal/api/vcsroots.go b/internal/api/vcsroots.go index 1b6de4f..7dd8275 100644 --- a/internal/api/vcsroots.go +++ b/internal/api/vcsroots.go @@ -156,9 +156,9 @@ func NewPerforceVcsRootProperties(port, user, password, stream string) PropertyL } // Helper: creates a property list for a Git VCS root -func NewGitVcsRootProperties(url, branch string) PropertyList { +func NewGitVcsRootProperties(repoURL, branch string) PropertyList { props := []Property{ - {Name: "url", Value: url}, + {Name: "url", Value: repoURL}, } if branch != "" { props = append(props, Property{Name: "branch", Value: branch}) diff --git a/internal/cmd/git_helpers_test.go b/internal/cmd/git_helpers_test.go index 4be90eb..97bcfa7 100644 --- a/internal/cmd/git_helpers_test.go +++ b/internal/cmd/git_helpers_test.go @@ -360,7 +360,7 @@ func TestLoadLocalChanges(t *testing.T) { _, err := loadLocalChanges("git") assert.Error(t, err) - assert.Contains(t, err.Error(), "no uncommitted changes") + assert.Contains(t, err.Error(), "no local changes found") }) t.Run("git source not in repo", func(t *testing.T) { @@ -370,7 +370,7 @@ func TestLoadLocalChanges(t *testing.T) { _, err := loadLocalChanges("git") assert.Error(t, err) - assert.Contains(t, err.Error(), "not a git repository") + assert.Contains(t, err.Error(), "no supported VCS detected") }) t.Run("file source", func(t *testing.T) { diff --git a/internal/cmd/run_lifecycle.go b/internal/cmd/run_lifecycle.go index 5c24b5c..a956df8 100644 --- a/internal/cmd/run_lifecycle.go +++ b/internal/cmd/run_lifecycle.go @@ -138,35 +138,35 @@ func runRunStart(jobID string, opts *runStartOptions) error { } var headCommit string - if opts.localChanges != "" && opts.branch == "" { + var personalChangeID string + var localPatch []byte + + if opts.localChanges != "" { vcs := DetectVCS() - if vcs == nil { - return tcerrors.WithSuggestion( - "no supported VCS detected", - "Run this command from within a git repository or Perforce workspace, or specify --branch explicitly", - ) - } - branch, err := vcs.GetCurrentBranch() - if err != nil { - return err + + if opts.branch == "" { + if vcs == nil { + return tcerrors.WithSuggestion( + "no supported VCS detected", + "Run this command from within a git repository or Perforce workspace, or specify --branch explicitly", + ) + } + branch, err := vcs.GetCurrentBranch() + if err != nil { + return err + } + opts.branch = branch + output.Info("Using current %s branch: %s", vcs.Name(), branch) } - opts.branch = branch - output.Info("Using current %s branch: %s", vcs.Name(), branch) - } - if opts.localChanges != "" && !opts.noPush { - vcs := DetectVCS() - if vcs != nil && !vcs.BranchExistsOnRemote(opts.branch) { + if !opts.noPush && vcs != nil && !vcs.BranchExistsOnRemote(opts.branch) { output.Info("Pushing branch to remote...") if err := vcs.PushBranch(opts.branch); err != nil { return err } output.Success("Branch pushed to remote") } - } - if opts.localChanges != "" { - vcs := DetectVCS() if vcs != nil { commit, err := vcs.GetHeadRevision() if err != nil { @@ -174,6 +174,13 @@ func runRunStart(jobID string, opts *runStartOptions) error { } headCommit = commit } + + patch, err := loadLocalChanges(opts.localChanges) + if err != nil { + return err + } + localPatch = patch + opts.personal = true } client, err := getClient() @@ -181,27 +188,18 @@ func runRunStart(jobID string, opts *runStartOptions) error { return err } - var personalChangeID string - if opts.localChanges != "" { - patch, err := loadLocalChanges(opts.localChanges) - if err != nil { - return err - } - + if localPatch != nil { output.Info("Uploading local changes...") description := opts.comment if description == "" { description = "Personal build with local changes" } - - changeID, err := client.UploadDiffChanges(patch, description) + changeID, err := client.UploadDiffChanges(localPatch, description) if err != nil { return fmt.Errorf("failed to upload changes: %w", err) } personalChangeID = changeID output.Success("Uploaded changes (ID: %s)", changeID) - - opts.personal = true } buildOpts := api.RunBuildOptions{ @@ -221,11 +219,6 @@ func runRunStart(jobID string, opts *runStartOptions) error { Revision: headCommit, } - // Wire up VCS-specific branch formatting when a provider is available - if vcs := DetectVCS(); vcs != nil { - buildOpts.VCSBranchFormatter = vcs.FormatVCSBranch - } - build, err := client.RunBuild(jobID, buildOpts) if err != nil { return err @@ -530,62 +523,6 @@ func (v *localChangesValue) Type() string { func loadLocalChanges(source string) ([]byte, error) { switch source { - case "git": - if !isGitRepo() { - return nil, tcerrors.WithSuggestion( - "not a git repository", - "Run this command from within a git repository, or use --local-changes to specify a diff file", - ) - } - patch, err := getGitDiff() - if err != nil { - return nil, err - } - if len(patch) == 0 { - return nil, tcerrors.WithSuggestion( - "no uncommitted changes found", - "Make some changes to your files before running a personal build, or use --local-changes to specify a diff file", - ) - } - return patch, nil - case "p4", "perforce": - p4 := &PerforceProvider{} - if !p4.IsAvailable() { - return nil, tcerrors.WithSuggestion( - "not a Perforce workspace", - "Run this command from within a Perforce workspace, or use --local-changes to specify a diff file", - ) - } - patch, err := p4.GetLocalDiff() - if err != nil { - return nil, err - } - if len(patch) == 0 { - return nil, tcerrors.WithSuggestion( - "no pending changes found", - "Open files for edit before running a personal build, or use --local-changes to specify a diff file", - ) - } - return patch, nil - case "auto", "": - vcs := DetectVCS() - if vcs == nil { - return nil, tcerrors.WithSuggestion( - "no supported VCS detected", - "Run this command from within a git repository or Perforce workspace, or use --local-changes to specify a diff file", - ) - } - patch, err := vcs.GetLocalDiff() - if err != nil { - return nil, err - } - if len(patch) == 0 { - return nil, tcerrors.WithSuggestion( - "no uncommitted changes found", - "Make some changes to your files before running a personal build, or use --local-changes to specify a diff file", - ) - } - return patch, nil case "-": patch, err := io.ReadAll(os.Stdin) if err != nil { @@ -598,6 +535,8 @@ func loadLocalChanges(source string) ([]byte, error) { ) } return patch, nil + case "git", "p4", "perforce", "auto": + return loadVCSDiff(source) default: patch, err := os.ReadFile(source) if err != nil { @@ -619,6 +558,32 @@ func loadLocalChanges(source string) ([]byte, error) { } } +func loadVCSDiff(source string) ([]byte, error) { + var vcs VCSProvider + if source == "auto" { + vcs = DetectVCS() + } else if p := DetectVCSByName(source); p != nil && p.IsAvailable() { + vcs = p + } + if vcs == nil { + return nil, tcerrors.WithSuggestion( + "no supported VCS detected", + "Run this command from within a git repository or Perforce workspace, or use --local-changes ", + ) + } + patch, err := vcs.GetLocalDiff() + if err != nil { + return nil, err + } + if len(patch) == 0 { + return nil, tcerrors.WithSuggestion( + "no local changes found", + "Make some changes before running a personal build, or use --local-changes ", + ) + } + return patch, nil +} + func getGitDiff() ([]byte, error) { untrackedFiles, err := getUntrackedFiles() if err != nil { diff --git a/internal/cmd/vcs_perforce.go b/internal/cmd/vcs_perforce.go index 17afc24..b4d8f7f 100644 --- a/internal/cmd/vcs_perforce.go +++ b/internal/cmd/vcs_perforce.go @@ -1,93 +1,66 @@ package cmd import ( + "context" "fmt" "os/exec" "strings" + "time" tcerrors "github.com/JetBrains/teamcity-cli/internal/errors" ) -// PerforceProvider implements VCSProvider for Perforce (Helix Core) workspaces. -type PerforceProvider struct{} +const p4Timeout = 10 * time.Second -func (p *PerforceProvider) Name() string { - return "perforce" +// p4Output runs a p4 command with a timeout and returns its stdout. +func p4Output(args ...string) ([]byte, error) { + ctx, cancel := context.WithTimeout(context.Background(), p4Timeout) + defer cancel() + return exec.CommandContext(ctx, "p4", args...).Output() } -func (p *PerforceProvider) IsAvailable() bool { - return isPerforceWorkspace() +// p4Run runs a p4 command with a timeout and returns any error. +func p4Run(args ...string) error { + ctx, cancel := context.WithTimeout(context.Background(), p4Timeout) + defer cancel() + return exec.CommandContext(ctx, "p4", args...).Run() } -func (p *PerforceProvider) GetCurrentBranch() (string, error) { - return getPerforceStream() -} - -func (p *PerforceProvider) GetHeadRevision() (string, error) { - return getPerforceChangelist() -} - -func (p *PerforceProvider) GetLocalDiff() ([]byte, error) { - return getPerforceDiff() -} - -func (p *PerforceProvider) BranchExistsOnRemote(branch string) bool { - // Perforce streams/depot paths always exist on the server if the workspace is valid. - // Check by querying the stream spec. - if strings.HasPrefix(branch, "//") { - cmd := exec.Command("p4", "stream", "-o", branch) - err := cmd.Run() - return err == nil +// p4ZtagField extracts a named field from p4 -ztag output. +func p4ZtagField(output []byte, field string) string { + prefix := "... " + field + " " + for _, line := range strings.Split(string(output), "\n") { + line = strings.TrimSpace(line) + if strings.HasPrefix(line, prefix) { + return strings.TrimPrefix(line, prefix) + } } - return true -} - -func (p *PerforceProvider) PushBranch(_ string) error { - // Perforce doesn't have a push concept; changes are submitted directly. - return nil -} - -func (p *PerforceProvider) FormatRevision(rev string) string { - // Perforce changelist numbers are typically short; return as-is. - return rev + return "" } -func (p *PerforceProvider) FormatVCSBranch(branch string) string { - // Perforce depot paths are used as-is in TeamCity VCS branch references. - return branch -} +// PerforceProvider implements VCSProvider for Perforce (Helix Core) workspaces. +type PerforceProvider struct{} -func (p *PerforceProvider) DiffHint(firstRev, lastRev string) string { - return fmt.Sprintf("p4 changes -l @%s,@%s", firstRev, lastRev) +func (p *PerforceProvider) Name() string { + return "perforce" } -// isPerforceWorkspace checks if the current directory is inside a Perforce workspace. -func isPerforceWorkspace() bool { - cmd := exec.Command("p4", "info") - out, err := cmd.Output() +func (p *PerforceProvider) IsAvailable() bool { + out, err := p4Output("info") if err != nil { return false } - // p4 info returns "Client unknown" when not in a valid workspace - outStr := string(out) - if strings.Contains(outStr, "Client unknown") { - return false - } - // Verify there's a Client root set - return strings.Contains(outStr, "Client root:") + s := string(out) + return !strings.Contains(s, "Client unknown") && strings.Contains(s, "Client root:") } -// getPerforceStream returns the current Perforce stream depot path. -// Falls back to the depot path if streams are not used. -func getPerforceStream() (string, error) { - // Try to get the stream from the current client spec +func (p *PerforceProvider) GetCurrentBranch() (string, error) { clientName, err := getPerforceClientName() if err != nil { return "", err } - cmd := exec.Command("p4", "-ztag", "client", "-o", clientName) - out, err := cmd.Output() + out, err := p4Output("-ztag", "client", "-o", clientName) if err != nil { return "", tcerrors.WithSuggestion( "failed to get Perforce client spec", @@ -95,26 +68,12 @@ func getPerforceStream() (string, error) { ) } - // Parse the stream field from ztag output - for _, line := range strings.Split(string(out), "\n") { - line = strings.TrimSpace(line) - if strings.HasPrefix(line, "... Stream ") { - stream := strings.TrimPrefix(line, "... Stream ") - return strings.TrimSpace(stream), nil - } + if stream := p4ZtagField(out, "Stream"); stream != "" { + return stream, nil } - - // If no stream, try to extract the depot path from the client mapping - for _, line := range strings.Split(string(out), "\n") { - line = strings.TrimSpace(line) - if strings.HasPrefix(line, "... View0 ") { - view := strings.TrimPrefix(line, "... View0 ") - // View format: "//depot/path/... //client/..." - parts := strings.Fields(view) - if len(parts) >= 1 { - depotPath := strings.TrimSuffix(parts[0], "/...") - return depotPath, nil - } + if view := p4ZtagField(out, "View0"); view != "" { + if parts := strings.Fields(view); len(parts) >= 1 { + return strings.TrimSuffix(parts[0], "/..."), nil } } @@ -124,41 +83,13 @@ func getPerforceStream() (string, error) { ) } -// getPerforceClientName returns the name of the current Perforce client/workspace. -func getPerforceClientName() (string, error) { - cmd := exec.Command("p4", "-ztag", "info") - out, err := cmd.Output() - if err != nil { - return "", tcerrors.WithSuggestion( - "failed to get Perforce info", - "Ensure p4 is installed and P4PORT/P4USER/P4CLIENT are configured", - ) - } - - for _, line := range strings.Split(string(out), "\n") { - line = strings.TrimSpace(line) - if strings.HasPrefix(line, "... clientName ") { - return strings.TrimPrefix(line, "... clientName "), nil - } - } - - return "", tcerrors.WithSuggestion( - "no Perforce client found", - "Set P4CLIENT or run 'p4 set P4CLIENT='", - ) -} - -// getPerforceChangelist returns the latest submitted changelist number -// that affects the current workspace. -func getPerforceChangelist() (string, error) { +func (p *PerforceProvider) GetHeadRevision() (string, error) { clientName, err := getPerforceClientName() if err != nil { return "", err } - // Get the latest changelist synced to this workspace - cmd := exec.Command("p4", "changes", "-m1", "-s", "submitted", "@"+clientName) - out, err := cmd.Output() + out, err := p4Output("changes", "-m1", "-s", "submitted", "@"+clientName) if err != nil { return "", tcerrors.WithSuggestion( "failed to get Perforce changelist", @@ -166,47 +97,34 @@ func getPerforceChangelist() (string, error) { ) } - // Output format: "Change 12345 on 2024/01/01 by user@client 'description'" - outStr := strings.TrimSpace(string(out)) - if outStr == "" { - return "", tcerrors.WithSuggestion( - "no submitted changelists found", - "Submit at least one changelist, or specify --branch explicitly", - ) - } - - fields := strings.Fields(outStr) + fields := strings.Fields(strings.TrimSpace(string(out))) if len(fields) >= 2 && fields[0] == "Change" { return fields[1], nil } return "", tcerrors.WithSuggestion( - "unexpected p4 changes output", - "Ensure p4 is working correctly", + "no submitted changelists found", + "Submit at least one changelist, or specify --branch explicitly", ) } -// getPerforceDiff generates a unified diff from pending changes in the default changelist. -func getPerforceDiff() ([]byte, error) { - // First check if there are any open files - openCmd := exec.Command("p4", "opened") - openOut, err := openCmd.Output() +func (p *PerforceProvider) GetLocalDiff() ([]byte, error) { + openOut, err := p4Output("opened") if err != nil { return nil, tcerrors.WithSuggestion( "failed to list opened files", "Ensure you are in a Perforce workspace with pending changes", ) } - if strings.TrimSpace(string(openOut)) == "" { - return nil, nil // No pending changes + return nil, nil } - // Generate unified diff of all opened files - cmd := exec.Command("p4", "diff", "-du") - out, err := cmd.Output() + ctx, cancel := context.WithTimeout(context.Background(), p4Timeout) + defer cancel() + out, err := exec.CommandContext(ctx, "p4", "diff", "-du").Output() if err != nil { - // p4 diff returns exit code 1 if there are differences (which is expected) + // p4 diff returns exit code 1 when differences exist (expected) if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { return out, nil } @@ -215,24 +133,43 @@ func getPerforceDiff() ([]byte, error) { "Ensure you have pending changes in your workspace", ) } - return out, nil } -// getPerforcePort returns the Perforce server address (P4PORT). -func getPerforcePort() (string, error) { - cmd := exec.Command("p4", "-ztag", "info") - out, err := cmd.Output() +func (p *PerforceProvider) BranchExistsOnRemote(_ string) bool { + // Perforce depot paths/streams always exist on the server; no push needed. + return true +} + +func (p *PerforceProvider) PushBranch(_ string) error { + return nil // Perforce doesn't have a push concept +} + +func (p *PerforceProvider) FormatRevision(rev string) string { + return rev +} + +func (p *PerforceProvider) FormatVCSBranch(branch string) string { + return branch +} + +func (p *PerforceProvider) DiffHint(firstRev, lastRev string) string { + return fmt.Sprintf("p4 changes -l @%s,@%s", firstRev, lastRev) +} + +func getPerforceClientName() (string, error) { + out, err := p4Output("-ztag", "info") if err != nil { - return "", err + return "", tcerrors.WithSuggestion( + "failed to get Perforce info", + "Ensure p4 is installed and P4PORT/P4USER/P4CLIENT are configured", + ) } - - for _, line := range strings.Split(string(out), "\n") { - line = strings.TrimSpace(line) - if strings.HasPrefix(line, "... serverAddress ") { - return strings.TrimPrefix(line, "... serverAddress "), nil - } + if name := p4ZtagField(out, "clientName"); name != "" { + return name, nil } - - return "", fmt.Errorf("P4PORT not found") + return "", tcerrors.WithSuggestion( + "no Perforce client found", + "Set P4CLIENT or run 'p4 set P4CLIENT='", + ) } From 736c8e3dc9fab0234466bb794de7b6c580f72563 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 7 Feb 2026 11:46:02 +0000 Subject: [PATCH 3/4] refactor: Strip verbose comments and dead code from Perforce support Remove AI-generated doc comments, unused p4Run function, and compact single-line interface delegations for cleaner review. https://claude.ai/code/session_01Buy3ZEvVTWwhkQHU4H442q --- internal/api/perforce_test.go | 46 +++----------------- internal/api/vcsroots.go | 33 +++----------- internal/api/vcsroots_test.go | 1 - internal/cmd/run_analysis.go | 3 +- internal/cmd/vcs_git.go | 38 ++++------------- internal/cmd/vcs_perforce.go | 37 +++------------- internal/cmd/vcs_provider.go | 38 +---------------- internal/cmd/vcs_provider_test.go | 71 +++++++------------------------ 8 files changed, 42 insertions(+), 225 deletions(-) diff --git a/internal/api/perforce_test.go b/internal/api/perforce_test.go index 6e4dadf..2db2f08 100644 --- a/internal/api/perforce_test.go +++ b/internal/api/perforce_test.go @@ -6,7 +6,6 @@ import ( "context" "fmt" "log" - "strings" "testing" "time" @@ -22,7 +21,6 @@ const ( p4dName = "tc-test-p4d" ) -// perforceTestEnv holds the state for Perforce integration tests type perforceTestEnv struct { container testcontainers.Container port string @@ -36,7 +34,6 @@ func (e *perforceTestEnv) Cleanup() { } } -// startP4D starts a Perforce server container and populates it with test data. func startP4D(ctx context.Context, networkName string) (*perforceTestEnv, error) { log.Println("Starting Perforce server (p4d)...") @@ -88,7 +85,6 @@ func startP4D(ctx context.Context, networkName string) (*perforceTestEnv, error) return env, nil } -// waitForP4D polls p4d until it accepts connections. func waitForP4D(ctx context.Context, container testcontainers.Container) error { deadline := time.After(30 * time.Second) for { @@ -107,14 +103,11 @@ func waitForP4D(ctx context.Context, container testcontainers.Container) error { } } -// populateP4Depot creates a test depot with files in the p4d container. func populateP4Depot(ctx context.Context, container testcontainers.Container) error { commands := [][]string{ - // Create a client workspace for initial setup {"bash", "-c", `p4 -p localhost:1666 -u admin client -o test-setup | sed 's|//depot/...|//depot/main/...|' | p4 -p localhost:1666 -u admin client -i`}, - // Create the depot structure with a test file {"bash", "-c", `export P4PORT=localhost:1666 P4USER=admin P4CLIENT=test-setup mkdir -p /tmp/p4ws/main cd /tmp/p4ws @@ -135,7 +128,6 @@ p4 -p localhost:1666 -u admin -c test-setup submit -d "Initial commit" 2>/dev/nu return nil } -// TestPerforceVcsRootCRUD tests creating, reading, and deleting a Perforce VCS root. func TestPerforceVcsRootCRUD(T *testing.T) { if testEnvRef == nil || testEnvRef.network == nil { T.Skip("test requires testcontainers with Docker network") @@ -151,16 +143,13 @@ func TestPerforceVcsRootCRUD(T *testing.T) { vcsRootID := "Sandbox_PerforceTest" T.Run("create perforce vcs root", func(t *testing.T) { - // Use the Docker network name for the P4 port so TeamCity server can reach it - p4Port := fmt.Sprintf("perforce-server:1666") - root, err := client.CreateVcsRoot(api.CreateVcsRootRequest{ - ID: vcsRootID, - Name: "Perforce Test Depot", - VcsName: "perforce", + ID: vcsRootID, + Name: "Perforce Test Depot", + VcsName: "perforce", ProjectID: testProject, Properties: api.NewPerforceVcsRootProperties( - p4Port, + "perforce-server:1666", "admin", "", "", @@ -169,7 +158,6 @@ func TestPerforceVcsRootCRUD(T *testing.T) { require.NoError(t, err) assert.Equal(t, vcsRootID, root.ID) assert.Equal(t, "perforce", root.VcsName) - t.Logf("Created VCS root: %s", root.ID) }) T.Run("get perforce vcs root", func(t *testing.T) { @@ -178,14 +166,12 @@ func TestPerforceVcsRootCRUD(T *testing.T) { assert.Equal(t, "perforce", root.VcsName) assert.Equal(t, "Perforce Test Depot", root.Name) - // Verify properties props := make(map[string]string) for _, p := range root.Properties.Property { props[p.Name] = p.Value } assert.Contains(t, props["port"], "perforce-server:1666") assert.Equal(t, "admin", props["user"]) - t.Logf("VCS root properties: %v", props) }) T.Run("list vcs roots includes perforce", func(t *testing.T) { @@ -209,7 +195,6 @@ func TestPerforceVcsRootCRUD(T *testing.T) { }) T.Run("attach to build config", func(t *testing.T) { - // Create a new build config for this test p4ConfigID := "Sandbox_PerforceDemo" if !client.BuildTypeExists(p4ConfigID) { _, err := client.CreateBuildType(testProject, api.CreateBuildTypeRequest{ @@ -221,15 +206,11 @@ func TestPerforceVcsRootCRUD(T *testing.T) { err := client.AttachVcsRoot(p4ConfigID, vcsRootID) require.NoError(t, err) - t.Logf("Attached VCS root %s to build config %s", vcsRootID, p4ConfigID) }) T.Run("delete perforce vcs root", func(t *testing.T) { - // First detach from build configs by deleting the build config - // (simpler than detaching VCS root entries) p4ConfigID := "Sandbox_PerforceDemo" if client.BuildTypeExists(p4ConfigID) { - // Delete the build config first to avoid "VCS root is in use" error raw, err := client.RawRequest("DELETE", fmt.Sprintf("/app/rest/buildTypes/id:%s", p4ConfigID), nil, nil) if err != nil { t.Logf("Warning: could not delete build config: %v", err) @@ -240,12 +221,10 @@ func TestPerforceVcsRootCRUD(T *testing.T) { err := client.DeleteVcsRoot(vcsRootID) require.NoError(t, err) - - assert.False(t, client.VcsRootExists(vcsRootID), "VCS root should be deleted") + assert.False(t, client.VcsRootExists(vcsRootID)) }) } -// TestPerforceBuildWithVcsRoot tests running a build that uses a Perforce VCS root. func TestPerforceBuildWithVcsRoot(T *testing.T) { if testEnvRef == nil || testEnvRef.network == nil { T.Skip("test requires testcontainers with Docker network") @@ -258,7 +237,6 @@ func TestPerforceBuildWithVcsRoot(T *testing.T) { } defer p4Env.Cleanup() - // Create a Perforce VCS root vcsRootID := "Sandbox_P4BuildTest" p4ConfigID := "Sandbox_P4BuildDemo" @@ -270,7 +248,6 @@ func TestPerforceBuildWithVcsRoot(T *testing.T) { client.DeleteVcsRoot(vcsRootID) } - // Create VCS root pointing to the p4d container root, err := client.CreateVcsRoot(api.CreateVcsRootRequest{ ID: vcsRootID, Name: "P4 Build Test", @@ -293,7 +270,6 @@ func TestPerforceBuildWithVcsRoot(T *testing.T) { client.DeleteVcsRoot(vcsRootID) }() - // Create build config with a simple step _, err = client.CreateBuildType(testProject, api.CreateBuildTypeRequest{ ID: p4ConfigID, Name: "P4 Build Demo", @@ -315,15 +291,12 @@ func TestPerforceBuildWithVcsRoot(T *testing.T) { }) require.NoError(T, err) - // Trigger a build build, err := client.RunBuild(p4ConfigID, api.RunBuildOptions{ Comment: "Perforce integration test", }) require.NoError(T, err) T.Logf("Triggered build #%d", build.ID) - // Wait for build to complete (it may fail if p4 checkout doesn't work, - // but the important thing is that the VCS root integration works) buildID := fmt.Sprintf("%d", build.ID) deadline := time.Now().Add(3 * time.Minute) for time.Now().Before(deadline) { @@ -337,14 +310,8 @@ func TestPerforceBuildWithVcsRoot(T *testing.T) { T.Logf("Build finished: state=%s status=%s", build.State, build.Status) - // Get build log to see what happened buildLog, err := client.GetBuildLog(buildID) if err == nil { - // Look for Perforce-related output in the log - if strings.Contains(buildLog, "Perforce") || strings.Contains(buildLog, "p4") { - T.Logf("Build log contains Perforce references") - } - // Log last 500 chars for debugging if len(buildLog) > 500 { T.Logf("Build log (tail):\n%s", buildLog[len(buildLog)-500:]) } else { @@ -352,15 +319,12 @@ func TestPerforceBuildWithVcsRoot(T *testing.T) { } } - // The build triggered successfully with a Perforce VCS root - that's the key assertion assert.Equal(T, "finished", build.State, "build should have finished") } -// TestPerforceUploadDiffChanges tests uploading a Perforce-style diff as personal changes. func TestPerforceUploadDiffChanges(T *testing.T) { T.Parallel() - // Perforce diffs in unified format should upload just like Git diffs patch := []byte(`--- a/depot/main/test.txt +++ b/depot/main/test.txt @@ -1 +1 @@ diff --git a/internal/api/vcsroots.go b/internal/api/vcsroots.go index 7dd8275..3457b1c 100644 --- a/internal/api/vcsroots.go +++ b/internal/api/vcsroots.go @@ -7,58 +7,46 @@ import ( "net/url" ) -// VcsRoot represents a TeamCity VCS root type VcsRoot struct { ID string `json:"id,omitempty"` Name string `json:"name,omitempty"` - VcsName string `json:"vcsName,omitempty"` // e.g. "jetbrains.git", "perforce" + VcsName string `json:"vcsName,omitempty"` // "jetbrains.git", "perforce" ProjectID string `json:"projectId,omitempty"` Href string `json:"href,omitempty"` Properties PropertyList `json:"properties,omitempty"` Project *Project `json:"project,omitempty"` } -// VcsRootList represents a list of VCS roots type VcsRootList struct { Count int `json:"count"` VcsRoots []VcsRoot `json:"vcs-root"` } -// VcsRootOptions represents options for listing VCS roots type VcsRootOptions struct { Project string Limit int } -// GetVcsRoots returns a list of VCS roots func (c *Client) GetVcsRoots(opts VcsRootOptions) (*VcsRootList, error) { locator := NewLocator(). Add("project", opts.Project). AddIntDefault("count", opts.Limit, 30) - path := fmt.Sprintf("/app/rest/vcs-roots?locator=%s", locator.Encode()) - var result VcsRootList - if err := c.get(path, &result); err != nil { + if err := c.get(fmt.Sprintf("/app/rest/vcs-roots?locator=%s", locator.Encode()), &result); err != nil { return nil, err } - return &result, nil } -// GetVcsRoot returns a single VCS root by ID func (c *Client) GetVcsRoot(id string) (*VcsRoot, error) { - path := fmt.Sprintf("/app/rest/vcs-roots/id:%s", url.PathEscape(id)) - var vcsRoot VcsRoot - if err := c.get(path, &vcsRoot); err != nil { + if err := c.get(fmt.Sprintf("/app/rest/vcs-roots/id:%s", url.PathEscape(id)), &vcsRoot); err != nil { return nil, err } - return &vcsRoot, nil } -// CreateVcsRootRequest represents a request to create a VCS root type CreateVcsRootRequest struct { ID string `json:"id,omitempty"` Name string `json:"name"` @@ -67,7 +55,6 @@ type CreateVcsRootRequest struct { Properties PropertyList `json:"properties,omitempty"` } -// createVcsRootPayload is the JSON format expected by the API type createVcsRootPayload struct { ID string `json:"id,omitempty"` Name string `json:"name"` @@ -76,12 +63,10 @@ type createVcsRootPayload struct { Properties PropertyList `json:"properties,omitempty"` } -// ProjectRef is a reference to a project by ID type ProjectRef struct { ID string `json:"id"` } -// CreateVcsRoot creates a new VCS root func (c *Client) CreateVcsRoot(req CreateVcsRootRequest) (*VcsRoot, error) { payload := createVcsRootPayload{ ID: req.ID, @@ -102,23 +87,18 @@ func (c *Client) CreateVcsRoot(req CreateVcsRootRequest) (*VcsRoot, error) { if err := c.post("/app/rest/vcs-roots", bytes.NewReader(body), &vcsRoot); err != nil { return nil, err } - return &vcsRoot, nil } -// DeleteVcsRoot deletes a VCS root by ID func (c *Client) DeleteVcsRoot(id string) error { - path := fmt.Sprintf("/app/rest/vcs-roots/id:%s", url.PathEscape(id)) - return c.doNoContent("DELETE", path, nil, "") + return c.doNoContent("DELETE", fmt.Sprintf("/app/rest/vcs-roots/id:%s", url.PathEscape(id)), nil, "") } -// VcsRootExists checks if a VCS root exists func (c *Client) VcsRootExists(id string) bool { _, err := c.GetVcsRoot(id) return err == nil } -// AttachVcsRoot attaches a VCS root to a build configuration func (c *Client) AttachVcsRoot(buildTypeID string, vcsRootID string) error { payload := struct { ID string `json:"id"` @@ -135,11 +115,9 @@ func (c *Client) AttachVcsRoot(buildTypeID string, vcsRootID string) error { return fmt.Errorf("failed to marshal request: %w", err) } - path := fmt.Sprintf("/app/rest/buildTypes/id:%s/vcs-root-entries", buildTypeID) - return c.doNoContent("POST", path, bytes.NewReader(body), "") + return c.doNoContent("POST", fmt.Sprintf("/app/rest/buildTypes/id:%s/vcs-root-entries", buildTypeID), bytes.NewReader(body), "") } -// Helper: creates a property list for a Perforce VCS root func NewPerforceVcsRootProperties(port, user, password, stream string) PropertyList { props := []Property{ {Name: "port", Value: port}, @@ -155,7 +133,6 @@ func NewPerforceVcsRootProperties(port, user, password, stream string) PropertyL return PropertyList{Property: props} } -// Helper: creates a property list for a Git VCS root func NewGitVcsRootProperties(repoURL, branch string) PropertyList { props := []Property{ {Name: "url", Value: repoURL}, diff --git a/internal/api/vcsroots_test.go b/internal/api/vcsroots_test.go index 25e3665..0f224b3 100644 --- a/internal/api/vcsroots_test.go +++ b/internal/api/vcsroots_test.go @@ -174,7 +174,6 @@ func TestCreateVcsRoot(T *testing.T) { assert.Equal(t, "perforce", payload.VcsName) assert.Equal(t, "P4 Stream", payload.Name) - // Verify Perforce-specific properties props := make(map[string]string) for _, p := range payload.Properties.Property { props[p.Name] = p.Value diff --git a/internal/cmd/run_analysis.go b/internal/cmd/run_analysis.go index a727390..d037312 100644 --- a/internal/cmd/run_analysis.go +++ b/internal/cmd/run_analysis.go @@ -60,10 +60,9 @@ func runRunChanges(runID string, opts *runChangesOptions) error { fmt.Printf("CHANGES (%d %s)\n\n", changes.Count, english.PluralWord(changes.Count, "commit", "commits")) - // Detect VCS provider for formatting (falls back to Git-style if unavailable) vcs := DetectVCS() if vcs == nil { - vcs = &GitProvider{} // default to Git formatting + vcs = &GitProvider{} } var firstRev, lastRev string diff --git a/internal/cmd/vcs_git.go b/internal/cmd/vcs_git.go index 1dca55e..7498c0c 100644 --- a/internal/cmd/vcs_git.go +++ b/internal/cmd/vcs_git.go @@ -5,36 +5,15 @@ import ( "strings" ) -// GitProvider implements VCSProvider for Git repositories. type GitProvider struct{} -func (g *GitProvider) Name() string { - return "git" -} - -func (g *GitProvider) IsAvailable() bool { - return isGitRepo() -} - -func (g *GitProvider) GetCurrentBranch() (string, error) { - return getCurrentBranch() -} - -func (g *GitProvider) GetHeadRevision() (string, error) { - return getHeadCommit() -} - -func (g *GitProvider) GetLocalDiff() ([]byte, error) { - return getGitDiff() -} - -func (g *GitProvider) BranchExistsOnRemote(branch string) bool { - return branchExistsOnRemote(branch) -} - -func (g *GitProvider) PushBranch(branch string) error { - return pushBranch(branch) -} +func (g *GitProvider) Name() string { return "git" } +func (g *GitProvider) IsAvailable() bool { return isGitRepo() } +func (g *GitProvider) GetCurrentBranch() (string, error) { return getCurrentBranch() } +func (g *GitProvider) GetHeadRevision() (string, error) { return getHeadCommit() } +func (g *GitProvider) GetLocalDiff() ([]byte, error) { return getGitDiff() } +func (g *GitProvider) BranchExistsOnRemote(b string) bool { return branchExistsOnRemote(b) } +func (g *GitProvider) PushBranch(b string) error { return pushBranch(b) } func (g *GitProvider) FormatRevision(rev string) string { if len(rev) > 7 { @@ -51,8 +30,7 @@ func (g *GitProvider) FormatVCSBranch(branch string) string { } func (g *GitProvider) DiffHint(firstRev, lastRev string) string { - first := firstRev - last := lastRev + first, last := firstRev, lastRev if len(first) > 7 { first = first[:7] } diff --git a/internal/cmd/vcs_perforce.go b/internal/cmd/vcs_perforce.go index b4d8f7f..dd9e7d7 100644 --- a/internal/cmd/vcs_perforce.go +++ b/internal/cmd/vcs_perforce.go @@ -12,21 +12,12 @@ import ( const p4Timeout = 10 * time.Second -// p4Output runs a p4 command with a timeout and returns its stdout. func p4Output(args ...string) ([]byte, error) { ctx, cancel := context.WithTimeout(context.Background(), p4Timeout) defer cancel() return exec.CommandContext(ctx, "p4", args...).Output() } -// p4Run runs a p4 command with a timeout and returns any error. -func p4Run(args ...string) error { - ctx, cancel := context.WithTimeout(context.Background(), p4Timeout) - defer cancel() - return exec.CommandContext(ctx, "p4", args...).Run() -} - -// p4ZtagField extracts a named field from p4 -ztag output. func p4ZtagField(output []byte, field string) string { prefix := "... " + field + " " for _, line := range strings.Split(string(output), "\n") { @@ -38,12 +29,9 @@ func p4ZtagField(output []byte, field string) string { return "" } -// PerforceProvider implements VCSProvider for Perforce (Helix Core) workspaces. type PerforceProvider struct{} -func (p *PerforceProvider) Name() string { - return "perforce" -} +func (p *PerforceProvider) Name() string { return "perforce" } func (p *PerforceProvider) IsAvailable() bool { out, err := p4Output("info") @@ -124,9 +112,8 @@ func (p *PerforceProvider) GetLocalDiff() ([]byte, error) { defer cancel() out, err := exec.CommandContext(ctx, "p4", "diff", "-du").Output() if err != nil { - // p4 diff returns exit code 1 when differences exist (expected) if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { - return out, nil + return out, nil // p4 diff returns exit 1 when differences exist } return nil, tcerrors.WithSuggestion( "failed to generate Perforce diff", @@ -136,22 +123,10 @@ func (p *PerforceProvider) GetLocalDiff() ([]byte, error) { return out, nil } -func (p *PerforceProvider) BranchExistsOnRemote(_ string) bool { - // Perforce depot paths/streams always exist on the server; no push needed. - return true -} - -func (p *PerforceProvider) PushBranch(_ string) error { - return nil // Perforce doesn't have a push concept -} - -func (p *PerforceProvider) FormatRevision(rev string) string { - return rev -} - -func (p *PerforceProvider) FormatVCSBranch(branch string) string { - return branch -} +func (p *PerforceProvider) BranchExistsOnRemote(_ string) bool { return true } +func (p *PerforceProvider) PushBranch(_ string) error { return nil } +func (p *PerforceProvider) FormatRevision(rev string) string { return rev } +func (p *PerforceProvider) FormatVCSBranch(branch string) string { return branch } func (p *PerforceProvider) DiffHint(firstRev, lastRev string) string { return fmt.Sprintf("p4 changes -l @%s,@%s", firstRev, lastRev) diff --git a/internal/cmd/vcs_provider.go b/internal/cmd/vcs_provider.go index 96ee3b9..d20c6e4 100644 --- a/internal/cmd/vcs_provider.go +++ b/internal/cmd/vcs_provider.go @@ -1,56 +1,21 @@ package cmd // VCSProvider abstracts version control operations used by the CLI. -// Implementations exist for Git and Perforce. type VCSProvider interface { - // Name returns the VCS type name (e.g., "git", "perforce"). Name() string - - // IsAvailable returns true if this VCS is detected in the current working directory. IsAvailable() bool - - // GetCurrentBranch returns the current branch or stream name. - // For Git, this is the branch name. For Perforce, this is the stream depot path. GetCurrentBranch() (string, error) - - // GetHeadRevision returns the current revision identifier. - // For Git, this is a commit SHA. For Perforce, this is a changelist number. GetHeadRevision() (string, error) - - // GetLocalDiff returns a unified diff of uncommitted local changes. - // For Git, this is `git diff HEAD` including untracked files. - // For Perforce, this is `p4 diff -du` of pending changelists. GetLocalDiff() ([]byte, error) - - // BranchExistsOnRemote checks whether the branch/stream exists on the remote server. BranchExistsOnRemote(branch string) bool - - // PushBranch pushes the given branch to the remote. - // For Perforce, this is a no-op (changes are submitted directly). PushBranch(branch string) error - - // FormatRevision returns a short display form of a revision identifier. - // For Git, truncates SHA to 7 chars. For Perforce, returns the changelist number as-is. FormatRevision(rev string) string - - // FormatVCSBranch converts a user-facing branch name to the VCS-specific ref format - // used in TeamCity API calls. - // For Git: "main" -> "refs/heads/main". For Perforce: returns depot path as-is. FormatVCSBranch(branch string) string - - // DiffHint returns a human-readable hint for viewing the full diff between two revisions. - // For Git: "git diff abc1234^..def5678". For Perforce: "p4 changes -l @123,@456". DiffHint(firstRev, lastRev string) string } -// DetectVCS returns the VCS provider for the current working directory, -// or nil if no supported VCS is detected. func DetectVCS() VCSProvider { - providers := []VCSProvider{ - &GitProvider{}, - &PerforceProvider{}, - } - for _, p := range providers { + for _, p := range []VCSProvider{&GitProvider{}, &PerforceProvider{}} { if p.IsAvailable() { return p } @@ -58,7 +23,6 @@ func DetectVCS() VCSProvider { return nil } -// DetectVCSByName returns the VCS provider matching the given name, or nil. func DetectVCSByName(name string) VCSProvider { switch name { case "git": diff --git a/internal/cmd/vcs_provider_test.go b/internal/cmd/vcs_provider_test.go index 5d5f31c..c26f95a 100644 --- a/internal/cmd/vcs_provider_test.go +++ b/internal/cmd/vcs_provider_test.go @@ -7,26 +7,18 @@ import ( ) func TestGitProvider_Name(t *testing.T) { - p := &GitProvider{} - assert.Equal(t, "git", p.Name()) + assert.Equal(t, "git", (&GitProvider{}).Name()) } func TestGitProvider_FormatRevision(t *testing.T) { p := &GitProvider{} - - // Long SHA gets truncated to 7 assert.Equal(t, "abc1234", p.FormatRevision("abc12345678901234567890123456789012345678")) - - // Short string stays the same assert.Equal(t, "abc", p.FormatRevision("abc")) - - // Exactly 7 chars stays the same assert.Equal(t, "abc1234", p.FormatRevision("abc1234")) } func TestGitProvider_FormatVCSBranch(t *testing.T) { p := &GitProvider{} - assert.Equal(t, "refs/heads/main", p.FormatVCSBranch("main")) assert.Equal(t, "refs/heads/feature/test", p.FormatVCSBranch("feature/test")) assert.Equal(t, "refs/tags/v1.0", p.FormatVCSBranch("refs/tags/v1.0")) @@ -34,10 +26,7 @@ func TestGitProvider_FormatVCSBranch(t *testing.T) { } func TestGitProvider_DiffHint(t *testing.T) { - p := &GitProvider{} - - hint := p.DiffHint("abc1234567890", "def1234567890") - assert.Equal(t, "git diff abc1234^..def1234", hint) + assert.Equal(t, "git diff abc1234^..def1234", (&GitProvider{}).DiffHint("abc1234567890", "def1234567890")) } func TestGitProvider_IsAvailable(t *testing.T) { @@ -45,18 +34,14 @@ func TestGitProvider_IsAvailable(t *testing.T) { dir := setupGitRepo(t) restore := chdir(t, dir) defer restore() - - p := &GitProvider{} - assert.True(t, p.IsAvailable()) + assert.True(t, (&GitProvider{}).IsAvailable()) }) t.Run("not in git repo", func(t *testing.T) { dir := t.TempDir() restore := chdir(t, dir) defer restore() - - p := &GitProvider{} - assert.False(t, p.IsAvailable()) + assert.False(t, (&GitProvider{}).IsAvailable()) }) } @@ -69,8 +54,7 @@ func TestGitProvider_GetCurrentBranch(t *testing.T) { runGit(t, dir, "add", ".") runGit(t, dir, "commit", "-m", "initial") - p := &GitProvider{} - branch, err := p.GetCurrentBranch() + branch, err := (&GitProvider{}).GetCurrentBranch() assert.NoError(t, err) assert.Contains(t, []string{"main", "master"}, branch) } @@ -84,8 +68,7 @@ func TestGitProvider_GetHeadRevision(t *testing.T) { runGit(t, dir, "add", ".") runGit(t, dir, "commit", "-m", "initial") - p := &GitProvider{} - rev, err := p.GetHeadRevision() + rev, err := (&GitProvider{}).GetHeadRevision() assert.NoError(t, err) assert.Regexp(t, "^[0-9a-f]{40}$", rev) } @@ -98,61 +81,43 @@ func TestGitProvider_GetLocalDiff(t *testing.T) { createFile(t, dir, "test.txt", "content") runGit(t, dir, "add", ".") runGit(t, dir, "commit", "-m", "initial") - - // Make changes createFile(t, dir, "test.txt", "modified") - p := &GitProvider{} - diff, err := p.GetLocalDiff() + diff, err := (&GitProvider{}).GetLocalDiff() assert.NoError(t, err) assert.Contains(t, string(diff), "modified") } func TestPerforceProvider_Name(t *testing.T) { - p := &PerforceProvider{} - assert.Equal(t, "perforce", p.Name()) + assert.Equal(t, "perforce", (&PerforceProvider{}).Name()) } func TestPerforceProvider_FormatRevision(t *testing.T) { p := &PerforceProvider{} - - // Perforce changelist numbers are returned as-is assert.Equal(t, "12345", p.FormatRevision("12345")) assert.Equal(t, "1", p.FormatRevision("1")) } func TestPerforceProvider_FormatVCSBranch(t *testing.T) { p := &PerforceProvider{} - - // Perforce depot paths pass through unchanged assert.Equal(t, "//depot/main", p.FormatVCSBranch("//depot/main")) assert.Equal(t, "//stream/dev", p.FormatVCSBranch("//stream/dev")) } func TestPerforceProvider_DiffHint(t *testing.T) { - p := &PerforceProvider{} - - hint := p.DiffHint("100", "200") - assert.Equal(t, "p4 changes -l @100,@200", hint) + assert.Equal(t, "p4 changes -l @100,@200", (&PerforceProvider{}).DiffHint("100", "200")) } func TestPerforceProvider_PushBranch(t *testing.T) { - // Push is a no-op for Perforce - p := &PerforceProvider{} - err := p.PushBranch("//depot/main") - assert.NoError(t, err) + assert.NoError(t, (&PerforceProvider{}).PushBranch("//depot/main")) } func TestPerforceProvider_IsAvailable(t *testing.T) { - // Without Perforce installed, should return false dir := t.TempDir() restore := chdir(t, dir) defer restore() - - p := &PerforceProvider{} - // In CI without p4 binary, this should be false - // We don't assert true/false since it depends on the environment - _ = p.IsAvailable() + // Just exercise the code path; result depends on whether p4 is installed + _ = (&PerforceProvider{}).IsAvailable() } func TestDetectVCS_Git(t *testing.T) { @@ -169,21 +134,18 @@ func TestDetectVCS_NoVCS(t *testing.T) { dir := t.TempDir() restore := chdir(t, dir) defer restore() - - vcs := DetectVCS() - assert.Nil(t, vcs) + assert.Nil(t, DetectVCS()) } func TestDetectVCSByName(t *testing.T) { - tests := []struct { + for _, tc := range []struct { name string expected string }{ {"git", "git"}, {"p4", "perforce"}, {"perforce", "perforce"}, - } - for _, tc := range tests { + } { t.Run(tc.name, func(t *testing.T) { vcs := DetectVCSByName(tc.name) assert.NotNil(t, vcs) @@ -192,7 +154,6 @@ func TestDetectVCSByName(t *testing.T) { } t.Run("unknown", func(t *testing.T) { - vcs := DetectVCSByName("svn") - assert.Nil(t, vcs) + assert.Nil(t, DetectVCSByName("svn")) }) } From 2f1d2507f4315460f924616a4e6f1f1369e1d6cb Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 23 Feb 2026 17:28:31 +0000 Subject: [PATCH 4/4] refactor: Remove unused VCS root CRUD and FormatVCSBranch Delete vcsroots.go (no command uses it), its 347-line test file, VCS root CRUD integration tests, FormatVCSBranch (builds.go handles branch formatting inline), and mock handlers. Keeps only what commands actually call: detection, branch/revision queries, diff generation, and display formatting. https://claude.ai/code/session_01Buy3ZEvVTWwhkQHU4H442q --- internal/api/interface.go | 8 - internal/api/perforce_test.go | 313 --------------------------- internal/api/vcsroots.go | 144 ------------- internal/api/vcsroots_test.go | 346 ------------------------------ internal/cmd/testutil_test.go | 31 --- internal/cmd/vcs_git.go | 12 +- internal/cmd/vcs_perforce.go | 3 +- internal/cmd/vcs_provider.go | 1 - internal/cmd/vcs_provider_test.go | 14 -- 9 files changed, 2 insertions(+), 870 deletions(-) delete mode 100644 internal/api/vcsroots.go delete mode 100644 internal/api/vcsroots_test.go diff --git a/internal/api/interface.go b/internal/api/interface.go index 8238b5e..fca9a09 100644 --- a/internal/api/interface.go +++ b/internal/api/interface.go @@ -102,14 +102,6 @@ type ClientInterface interface { RemoveProjectFromPool(poolID int, projectID string) error SetAgentPool(agentID int, poolID int) error - // VCS Roots - GetVcsRoots(opts VcsRootOptions) (*VcsRootList, error) - GetVcsRoot(id string) (*VcsRoot, error) - CreateVcsRoot(req CreateVcsRootRequest) (*VcsRoot, error) - DeleteVcsRoot(id string) error - VcsRootExists(id string) bool - AttachVcsRoot(buildTypeID string, vcsRootID string) error - // Raw API access RawRequest(method, path string, body io.Reader, headers map[string]string) (*RawResponse, error) } diff --git a/internal/api/perforce_test.go b/internal/api/perforce_test.go index 2db2f08..eddde42 100644 --- a/internal/api/perforce_test.go +++ b/internal/api/perforce_test.go @@ -3,325 +3,12 @@ package api_test import ( - "context" - "fmt" - "log" "testing" - "time" - "github.com/JetBrains/teamcity-cli/internal/api" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/testcontainers/testcontainers-go" - "github.com/testcontainers/testcontainers-go/wait" ) -const ( - p4dImage = "sourcegraph/helix-p4d:latest" - p4dName = "tc-test-p4d" -) - -type perforceTestEnv struct { - container testcontainers.Container - port string - host string - ctx context.Context -} - -func (e *perforceTestEnv) Cleanup() { - if e.container != nil { - _ = e.container.Terminate(e.ctx) - } -} - -func startP4D(ctx context.Context, networkName string) (*perforceTestEnv, error) { - log.Println("Starting Perforce server (p4d)...") - - aliases := map[string][]string{} - var networks []string - if networkName != "" { - networks = []string{networkName} - aliases[networkName] = []string{"perforce-server"} - } - - container, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{ - ContainerRequest: testcontainers.ContainerRequest{ - Name: p4dName, - Image: p4dImage, - ExposedPorts: []string{"1666/tcp"}, - Networks: networks, - NetworkAliases: aliases, - WaitingFor: wait.ForLog("p4d -r"). - WithStartupTimeout(2 * time.Minute), - }, - Started: true, - }) - if err != nil { - return nil, fmt.Errorf("start p4d: %w", err) - } - - host, _ := container.Host(ctx) - port, _ := container.MappedPort(ctx, "1666/tcp") - - env := &perforceTestEnv{ - container: container, - host: host, - port: port.Port(), - ctx: ctx, - } - - log.Printf("P4D running at %s:%s", host, env.port) - - if err := waitForP4D(ctx, container); err != nil { - env.Cleanup() - return nil, fmt.Errorf("p4d not ready: %w", err) - } - - if err := populateP4Depot(ctx, container); err != nil { - env.Cleanup() - return nil, fmt.Errorf("populate depot: %w", err) - } - - return env, nil -} - -func waitForP4D(ctx context.Context, container testcontainers.Container) error { - deadline := time.After(30 * time.Second) - for { - select { - case <-deadline: - return fmt.Errorf("timeout waiting for p4d to accept connections") - case <-ctx.Done(): - return ctx.Err() - default: - _, _, err := container.Exec(ctx, []string{"p4", "-p", "localhost:1666", "info"}) - if err == nil { - return nil - } - time.Sleep(500 * time.Millisecond) - } - } -} - -func populateP4Depot(ctx context.Context, container testcontainers.Container) error { - commands := [][]string{ - {"bash", "-c", `p4 -p localhost:1666 -u admin client -o test-setup | -sed 's|//depot/...|//depot/main/...|' | -p4 -p localhost:1666 -u admin client -i`}, - {"bash", "-c", `export P4PORT=localhost:1666 P4USER=admin P4CLIENT=test-setup -mkdir -p /tmp/p4ws/main -cd /tmp/p4ws -p4 set P4PORT=localhost:1666 -p4 set P4USER=admin -p4 set P4CLIENT=test-setup -echo 'Hello from Perforce' > /tmp/p4ws/main/test.txt -p4 -p localhost:1666 -u admin -c test-setup add /tmp/p4ws/main/test.txt 2>/dev/null || true -p4 -p localhost:1666 -u admin -c test-setup submit -d "Initial commit" 2>/dev/null || true`}, - } - - for _, cmd := range commands { - _, _, err := container.Exec(ctx, cmd) - if err != nil { - return fmt.Errorf("p4d setup failed: %w", err) - } - } - return nil -} - -func TestPerforceVcsRootCRUD(T *testing.T) { - if testEnvRef == nil || testEnvRef.network == nil { - T.Skip("test requires testcontainers with Docker network") - } - - ctx := context.Background() - p4Env, err := startP4D(ctx, testEnvRef.network.Name) - if err != nil { - T.Skipf("could not start p4d: %v", err) - } - defer p4Env.Cleanup() - - vcsRootID := "Sandbox_PerforceTest" - - T.Run("create perforce vcs root", func(t *testing.T) { - root, err := client.CreateVcsRoot(api.CreateVcsRootRequest{ - ID: vcsRootID, - Name: "Perforce Test Depot", - VcsName: "perforce", - ProjectID: testProject, - Properties: api.NewPerforceVcsRootProperties( - "perforce-server:1666", - "admin", - "", - "", - ), - }) - require.NoError(t, err) - assert.Equal(t, vcsRootID, root.ID) - assert.Equal(t, "perforce", root.VcsName) - }) - - T.Run("get perforce vcs root", func(t *testing.T) { - root, err := client.GetVcsRoot(vcsRootID) - require.NoError(t, err) - assert.Equal(t, "perforce", root.VcsName) - assert.Equal(t, "Perforce Test Depot", root.Name) - - props := make(map[string]string) - for _, p := range root.Properties.Property { - props[p.Name] = p.Value - } - assert.Contains(t, props["port"], "perforce-server:1666") - assert.Equal(t, "admin", props["user"]) - }) - - T.Run("list vcs roots includes perforce", func(t *testing.T) { - roots, err := client.GetVcsRoots(api.VcsRootOptions{Project: testProject}) - require.NoError(t, err) - - found := false - for _, r := range roots.VcsRoots { - if r.ID == vcsRootID { - found = true - assert.Equal(t, "perforce", r.VcsName) - break - } - } - assert.True(t, found, "should find perforce VCS root in list") - }) - - T.Run("vcs root exists", func(t *testing.T) { - assert.True(t, client.VcsRootExists(vcsRootID)) - assert.False(t, client.VcsRootExists("NonExistent_P4Root")) - }) - - T.Run("attach to build config", func(t *testing.T) { - p4ConfigID := "Sandbox_PerforceDemo" - if !client.BuildTypeExists(p4ConfigID) { - _, err := client.CreateBuildType(testProject, api.CreateBuildTypeRequest{ - ID: p4ConfigID, - Name: "Perforce Demo", - }) - require.NoError(t, err) - } - - err := client.AttachVcsRoot(p4ConfigID, vcsRootID) - require.NoError(t, err) - }) - - T.Run("delete perforce vcs root", func(t *testing.T) { - p4ConfigID := "Sandbox_PerforceDemo" - if client.BuildTypeExists(p4ConfigID) { - raw, err := client.RawRequest("DELETE", fmt.Sprintf("/app/rest/buildTypes/id:%s", p4ConfigID), nil, nil) - if err != nil { - t.Logf("Warning: could not delete build config: %v", err) - } else if raw.StatusCode >= 300 { - t.Logf("Warning: delete build config returned %d", raw.StatusCode) - } - } - - err := client.DeleteVcsRoot(vcsRootID) - require.NoError(t, err) - assert.False(t, client.VcsRootExists(vcsRootID)) - }) -} - -func TestPerforceBuildWithVcsRoot(T *testing.T) { - if testEnvRef == nil || testEnvRef.network == nil { - T.Skip("test requires testcontainers with Docker network") - } - - ctx := context.Background() - p4Env, err := startP4D(ctx, testEnvRef.network.Name) - if err != nil { - T.Skipf("could not start p4d: %v", err) - } - defer p4Env.Cleanup() - - vcsRootID := "Sandbox_P4BuildTest" - p4ConfigID := "Sandbox_P4BuildDemo" - - // Cleanup from any previous run - if client.BuildTypeExists(p4ConfigID) { - client.RawRequest("DELETE", fmt.Sprintf("/app/rest/buildTypes/id:%s", p4ConfigID), nil, nil) - } - if client.VcsRootExists(vcsRootID) { - client.DeleteVcsRoot(vcsRootID) - } - - root, err := client.CreateVcsRoot(api.CreateVcsRootRequest{ - ID: vcsRootID, - Name: "P4 Build Test", - VcsName: "perforce", - ProjectID: testProject, - Properties: api.NewPerforceVcsRootProperties( - "perforce-server:1666", - "admin", - "", - "", - ), - }) - require.NoError(T, err) - T.Logf("Created VCS root: %s", root.ID) - - defer func() { - if client.BuildTypeExists(p4ConfigID) { - client.RawRequest("DELETE", fmt.Sprintf("/app/rest/buildTypes/id:%s", p4ConfigID), nil, nil) - } - client.DeleteVcsRoot(vcsRootID) - }() - - _, err = client.CreateBuildType(testProject, api.CreateBuildTypeRequest{ - ID: p4ConfigID, - Name: "P4 Build Demo", - }) - require.NoError(T, err) - - err = client.AttachVcsRoot(p4ConfigID, vcsRootID) - require.NoError(T, err) - - err = client.CreateBuildStep(p4ConfigID, api.BuildStep{ - Name: "Test P4", - Type: "simpleRunner", - Properties: api.PropertyList{ - Property: []api.Property{ - {Name: "script.content", Value: "echo 'Build from Perforce depot'\nls -la"}, - {Name: "use.custom.script", Value: "true"}, - }, - }, - }) - require.NoError(T, err) - - build, err := client.RunBuild(p4ConfigID, api.RunBuildOptions{ - Comment: "Perforce integration test", - }) - require.NoError(T, err) - T.Logf("Triggered build #%d", build.ID) - - buildID := fmt.Sprintf("%d", build.ID) - deadline := time.Now().Add(3 * time.Minute) - for time.Now().Before(deadline) { - build, err = client.GetBuild(buildID) - require.NoError(T, err) - if build.State == "finished" { - break - } - time.Sleep(5 * time.Second) - } - - T.Logf("Build finished: state=%s status=%s", build.State, build.Status) - - buildLog, err := client.GetBuildLog(buildID) - if err == nil { - if len(buildLog) > 500 { - T.Logf("Build log (tail):\n%s", buildLog[len(buildLog)-500:]) - } else { - T.Logf("Build log:\n%s", buildLog) - } - } - - assert.Equal(T, "finished", build.State, "build should have finished") -} - func TestPerforceUploadDiffChanges(T *testing.T) { T.Parallel() diff --git a/internal/api/vcsroots.go b/internal/api/vcsroots.go deleted file mode 100644 index 3457b1c..0000000 --- a/internal/api/vcsroots.go +++ /dev/null @@ -1,144 +0,0 @@ -package api - -import ( - "bytes" - "encoding/json" - "fmt" - "net/url" -) - -type VcsRoot struct { - ID string `json:"id,omitempty"` - Name string `json:"name,omitempty"` - VcsName string `json:"vcsName,omitempty"` // "jetbrains.git", "perforce" - ProjectID string `json:"projectId,omitempty"` - Href string `json:"href,omitempty"` - Properties PropertyList `json:"properties,omitempty"` - Project *Project `json:"project,omitempty"` -} - -type VcsRootList struct { - Count int `json:"count"` - VcsRoots []VcsRoot `json:"vcs-root"` -} - -type VcsRootOptions struct { - Project string - Limit int -} - -func (c *Client) GetVcsRoots(opts VcsRootOptions) (*VcsRootList, error) { - locator := NewLocator(). - Add("project", opts.Project). - AddIntDefault("count", opts.Limit, 30) - - var result VcsRootList - if err := c.get(fmt.Sprintf("/app/rest/vcs-roots?locator=%s", locator.Encode()), &result); err != nil { - return nil, err - } - return &result, nil -} - -func (c *Client) GetVcsRoot(id string) (*VcsRoot, error) { - var vcsRoot VcsRoot - if err := c.get(fmt.Sprintf("/app/rest/vcs-roots/id:%s", url.PathEscape(id)), &vcsRoot); err != nil { - return nil, err - } - return &vcsRoot, nil -} - -type CreateVcsRootRequest struct { - ID string `json:"id,omitempty"` - Name string `json:"name"` - VcsName string `json:"vcsName"` // "jetbrains.git", "perforce" - ProjectID string `json:"project,omitempty"` - Properties PropertyList `json:"properties,omitempty"` -} - -type createVcsRootPayload struct { - ID string `json:"id,omitempty"` - Name string `json:"name"` - VcsName string `json:"vcsName"` - Project *ProjectRef `json:"project,omitempty"` - Properties PropertyList `json:"properties,omitempty"` -} - -type ProjectRef struct { - ID string `json:"id"` -} - -func (c *Client) CreateVcsRoot(req CreateVcsRootRequest) (*VcsRoot, error) { - payload := createVcsRootPayload{ - ID: req.ID, - Name: req.Name, - VcsName: req.VcsName, - Properties: req.Properties, - } - if req.ProjectID != "" { - payload.Project = &ProjectRef{ID: req.ProjectID} - } - - body, err := json.Marshal(payload) - if err != nil { - return nil, fmt.Errorf("failed to marshal request: %w", err) - } - - var vcsRoot VcsRoot - if err := c.post("/app/rest/vcs-roots", bytes.NewReader(body), &vcsRoot); err != nil { - return nil, err - } - return &vcsRoot, nil -} - -func (c *Client) DeleteVcsRoot(id string) error { - return c.doNoContent("DELETE", fmt.Sprintf("/app/rest/vcs-roots/id:%s", url.PathEscape(id)), nil, "") -} - -func (c *Client) VcsRootExists(id string) bool { - _, err := c.GetVcsRoot(id) - return err == nil -} - -func (c *Client) AttachVcsRoot(buildTypeID string, vcsRootID string) error { - payload := struct { - ID string `json:"id"` - VcsRoot struct { - ID string `json:"id"` - } `json:"vcs-root"` - }{ - ID: vcsRootID, - } - payload.VcsRoot.ID = vcsRootID - - body, err := json.Marshal(payload) - if err != nil { - return fmt.Errorf("failed to marshal request: %w", err) - } - - return c.doNoContent("POST", fmt.Sprintf("/app/rest/buildTypes/id:%s/vcs-root-entries", buildTypeID), bytes.NewReader(body), "") -} - -func NewPerforceVcsRootProperties(port, user, password, stream string) PropertyList { - props := []Property{ - {Name: "port", Value: port}, - {Name: "user", Value: user}, - } - if password != "" { - props = append(props, Property{Name: "secure:passwd", Value: password}) - } - if stream != "" { - props = append(props, Property{Name: "stream", Value: stream}, - Property{Name: "use-client", Value: "false"}) - } - return PropertyList{Property: props} -} - -func NewGitVcsRootProperties(repoURL, branch string) PropertyList { - props := []Property{ - {Name: "url", Value: repoURL}, - } - if branch != "" { - props = append(props, Property{Name: "branch", Value: branch}) - } - return PropertyList{Property: props} -} diff --git a/internal/api/vcsroots_test.go b/internal/api/vcsroots_test.go deleted file mode 100644 index 0f224b3..0000000 --- a/internal/api/vcsroots_test.go +++ /dev/null @@ -1,346 +0,0 @@ -package api - -import ( - "encoding/json" - "io" - "net/http" - "net/http/httptest" - "strings" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestGetVcsRoots(T *testing.T) { - T.Parallel() - - T.Run("success", func(t *testing.T) { - t.Parallel() - - client := setupTestServer(t, func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, "GET", r.Method) - assert.True(t, strings.HasPrefix(r.URL.Path, "/app/rest/vcs-roots")) - w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(VcsRootList{ - Count: 2, - VcsRoots: []VcsRoot{ - {ID: "Project_GitRepo", Name: "Git Repo", VcsName: "jetbrains.git"}, - {ID: "Project_P4Depot", Name: "Perforce Depot", VcsName: "perforce"}, - }, - }) - }) - - roots, err := client.GetVcsRoots(VcsRootOptions{Project: "TestProject"}) - require.NoError(t, err) - assert.Equal(t, 2, roots.Count) - assert.Equal(t, "jetbrains.git", roots.VcsRoots[0].VcsName) - assert.Equal(t, "perforce", roots.VcsRoots[1].VcsName) - }) - - T.Run("empty", func(t *testing.T) { - t.Parallel() - - client := setupTestServer(t, func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(VcsRootList{Count: 0, VcsRoots: nil}) - }) - - roots, err := client.GetVcsRoots(VcsRootOptions{}) - require.NoError(t, err) - assert.Equal(t, 0, roots.Count) - }) -} - -func TestGetVcsRoot(T *testing.T) { - T.Parallel() - - T.Run("git root", func(t *testing.T) { - t.Parallel() - - client := setupTestServer(t, func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, "GET", r.Method) - w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(VcsRoot{ - ID: "Project_GitRepo", - Name: "Git Repository", - VcsName: "jetbrains.git", - Properties: PropertyList{ - Property: []Property{ - {Name: "url", Value: "https://github.com/example/repo.git"}, - {Name: "branch", Value: "refs/heads/main"}, - }, - }, - }) - }) - - root, err := client.GetVcsRoot("Project_GitRepo") - require.NoError(t, err) - assert.Equal(t, "jetbrains.git", root.VcsName) - assert.Len(t, root.Properties.Property, 2) - }) - - T.Run("perforce root", func(t *testing.T) { - t.Parallel() - - client := setupTestServer(t, func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(VcsRoot{ - ID: "Project_P4Depot", - Name: "Perforce Stream", - VcsName: "perforce", - Properties: PropertyList{ - Property: []Property{ - {Name: "port", Value: "ssl:perforce.example.com:1666"}, - {Name: "user", Value: "buildbot"}, - {Name: "stream", Value: "//depot/main"}, - }, - }, - }) - }) - - root, err := client.GetVcsRoot("Project_P4Depot") - require.NoError(t, err) - assert.Equal(t, "perforce", root.VcsName) - assert.Len(t, root.Properties.Property, 3) - }) - - T.Run("not found", func(t *testing.T) { - t.Parallel() - - client := setupTestServer(t, func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusNotFound) - w.Write([]byte(`{"errors":[{"message":"VCS root not found"}]}`)) - }) - - _, err := client.GetVcsRoot("NonExistent") - assert.Error(t, err) - }) -} - -func TestCreateVcsRoot(T *testing.T) { - T.Parallel() - - T.Run("create git root", func(t *testing.T) { - t.Parallel() - - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, "POST", r.Method) - assert.Equal(t, "/app/rest/vcs-roots", r.URL.Path) - - body, _ := io.ReadAll(r.Body) - var payload createVcsRootPayload - require.NoError(t, json.Unmarshal(body, &payload)) - - assert.Equal(t, "jetbrains.git", payload.VcsName) - assert.Equal(t, "My Git Repo", payload.Name) - assert.NotNil(t, payload.Project) - assert.Equal(t, "TestProject", payload.Project.ID) - - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(VcsRoot{ - ID: "TestProject_MyGitRepo", - Name: "My Git Repo", - VcsName: "jetbrains.git", - }) - })) - t.Cleanup(server.Close) - client := NewClient(server.URL, "test-token") - - root, err := client.CreateVcsRoot(CreateVcsRootRequest{ - Name: "My Git Repo", - VcsName: "jetbrains.git", - ProjectID: "TestProject", - Properties: NewGitVcsRootProperties( - "https://github.com/example/repo.git", - "refs/heads/main", - ), - }) - require.NoError(t, err) - assert.Equal(t, "TestProject_MyGitRepo", root.ID) - }) - - T.Run("create perforce root", func(t *testing.T) { - t.Parallel() - - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, "POST", r.Method) - - body, _ := io.ReadAll(r.Body) - var payload createVcsRootPayload - require.NoError(t, json.Unmarshal(body, &payload)) - - assert.Equal(t, "perforce", payload.VcsName) - assert.Equal(t, "P4 Stream", payload.Name) - - props := make(map[string]string) - for _, p := range payload.Properties.Property { - props[p.Name] = p.Value - } - assert.Equal(t, "ssl:p4.example.com:1666", props["port"]) - assert.Equal(t, "buildbot", props["user"]) - assert.Equal(t, "//depot/main", props["stream"]) - - w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(VcsRoot{ - ID: "TestProject_P4Stream", - Name: "P4 Stream", - VcsName: "perforce", - }) - })) - t.Cleanup(server.Close) - client := NewClient(server.URL, "test-token") - - root, err := client.CreateVcsRoot(CreateVcsRootRequest{ - Name: "P4 Stream", - VcsName: "perforce", - ProjectID: "TestProject", - Properties: NewPerforceVcsRootProperties( - "ssl:p4.example.com:1666", - "buildbot", - "secret123", - "//depot/main", - ), - }) - require.NoError(t, err) - assert.Equal(t, "perforce", root.VcsName) - }) -} - -func TestDeleteVcsRoot(T *testing.T) { - T.Parallel() - - T.Run("success", func(t *testing.T) { - t.Parallel() - - client := setupTestServer(t, func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, "DELETE", r.Method) - assert.True(t, strings.Contains(r.URL.Path, "/vcs-roots/id:")) - w.WriteHeader(http.StatusNoContent) - }) - - err := client.DeleteVcsRoot("TestProject_GitRepo") - require.NoError(t, err) - }) - - T.Run("not found", func(t *testing.T) { - t.Parallel() - - client := setupTestServer(t, func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusNotFound) - w.Write([]byte(`{"errors":[{"message":"VCS root not found"}]}`)) - }) - - err := client.DeleteVcsRoot("NonExistent") - assert.Error(t, err) - }) -} - -func TestVcsRootExists(T *testing.T) { - T.Parallel() - - T.Run("exists", func(t *testing.T) { - t.Parallel() - - client := setupTestServer(t, func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(VcsRoot{ID: "TestVcsRoot", Name: "Test"}) - }) - - assert.True(t, client.VcsRootExists("TestVcsRoot")) - }) - - T.Run("not exists", func(t *testing.T) { - t.Parallel() - - client := setupTestServer(t, func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusNotFound) - }) - - assert.False(t, client.VcsRootExists("NonExistent")) - }) -} - -func TestAttachVcsRoot(T *testing.T) { - T.Parallel() - - T.Run("success", func(t *testing.T) { - t.Parallel() - - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, "POST", r.Method) - assert.True(t, strings.HasSuffix(r.URL.Path, "/vcs-root-entries")) - - body, _ := io.ReadAll(r.Body) - assert.Contains(t, string(body), "TestProject_P4Root") - - w.WriteHeader(http.StatusOK) - })) - t.Cleanup(server.Close) - client := NewClient(server.URL, "test-token") - - err := client.AttachVcsRoot("Sandbox_Build", "TestProject_P4Root") - require.NoError(t, err) - }) -} - -func TestNewPerforceVcsRootProperties(T *testing.T) { - T.Parallel() - - T.Run("with stream", func(t *testing.T) { - t.Parallel() - - props := NewPerforceVcsRootProperties("p4.example.com:1666", "builder", "pass", "//depot/main") - - names := make(map[string]string) - for _, p := range props.Property { - names[p.Name] = p.Value - } - assert.Equal(t, "p4.example.com:1666", names["port"]) - assert.Equal(t, "builder", names["user"]) - assert.Equal(t, "pass", names["secure:passwd"]) - assert.Equal(t, "//depot/main", names["stream"]) - assert.Equal(t, "false", names["use-client"]) - }) - - T.Run("without password", func(t *testing.T) { - t.Parallel() - - props := NewPerforceVcsRootProperties("p4:1666", "user", "", "//depot/dev") - - names := make(map[string]string) - for _, p := range props.Property { - names[p.Name] = p.Value - } - assert.Equal(t, "p4:1666", names["port"]) - _, hasPassword := names["secure:passwd"] - assert.False(t, hasPassword) - }) -} - -func TestNewGitVcsRootProperties(T *testing.T) { - T.Parallel() - - T.Run("with branch", func(t *testing.T) { - t.Parallel() - - props := NewGitVcsRootProperties("https://github.com/example/repo.git", "refs/heads/main") - - names := make(map[string]string) - for _, p := range props.Property { - names[p.Name] = p.Value - } - assert.Equal(t, "https://github.com/example/repo.git", names["url"]) - assert.Equal(t, "refs/heads/main", names["branch"]) - }) - - T.Run("without branch", func(t *testing.T) { - t.Parallel() - - props := NewGitVcsRootProperties("https://github.com/example/repo.git", "") - - assert.Len(t, props.Property, 1) - assert.Equal(t, "url", props.Property[0].Name) - }) -} diff --git a/internal/cmd/testutil_test.go b/internal/cmd/testutil_test.go index 19f38f8..585a362 100644 --- a/internal/cmd/testutil_test.go +++ b/internal/cmd/testutil_test.go @@ -582,37 +582,6 @@ func setupMockClient(t *testing.T) *TestServer { w.WriteHeader(http.StatusNoContent) }) - // VCS Roots - ts.Handle("GET /app/rest/vcs-roots", func(w http.ResponseWriter, r *http.Request) { - JSON(w, api.VcsRootList{ - Count: 2, - VcsRoots: []api.VcsRoot{ - {ID: "TestProject_GitRepo", Name: "Git Repository", VcsName: "jetbrains.git"}, - {ID: "TestProject_P4Depot", Name: "Perforce Depot", VcsName: "perforce"}, - }, - }) - }) - - ts.Handle("GET /app/rest/vcs-roots/id:", func(w http.ResponseWriter, r *http.Request) { - JSON(w, api.VcsRoot{ - ID: "TestProject_GitRepo", - Name: "Git Repository", - VcsName: "jetbrains.git", - }) - }) - - ts.Handle("POST /app/rest/vcs-roots", func(w http.ResponseWriter, r *http.Request) { - JSON(w, api.VcsRoot{ - ID: "TestProject_NewRoot", - Name: "New Root", - VcsName: "perforce", - }) - }) - - ts.Handle("DELETE /app/rest/vcs-roots/id:", func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusNoContent) - }) - // Versioned Settings ts.Handle("GET /app/rest/projects/TestProject/versionedSettings/config", func(w http.ResponseWriter, r *http.Request) { JSON(w, api.VersionedSettingsConfig{ diff --git a/internal/cmd/vcs_git.go b/internal/cmd/vcs_git.go index 7498c0c..9b41bd2 100644 --- a/internal/cmd/vcs_git.go +++ b/internal/cmd/vcs_git.go @@ -1,9 +1,6 @@ package cmd -import ( - "fmt" - "strings" -) +import "fmt" type GitProvider struct{} @@ -22,13 +19,6 @@ func (g *GitProvider) FormatRevision(rev string) string { return rev } -func (g *GitProvider) FormatVCSBranch(branch string) string { - if branch != "" && !strings.HasPrefix(branch, "refs/") { - return "refs/heads/" + branch - } - return branch -} - func (g *GitProvider) DiffHint(firstRev, lastRev string) string { first, last := firstRev, lastRev if len(first) > 7 { diff --git a/internal/cmd/vcs_perforce.go b/internal/cmd/vcs_perforce.go index dd9e7d7..1889064 100644 --- a/internal/cmd/vcs_perforce.go +++ b/internal/cmd/vcs_perforce.go @@ -125,8 +125,7 @@ func (p *PerforceProvider) GetLocalDiff() ([]byte, error) { func (p *PerforceProvider) BranchExistsOnRemote(_ string) bool { return true } func (p *PerforceProvider) PushBranch(_ string) error { return nil } -func (p *PerforceProvider) FormatRevision(rev string) string { return rev } -func (p *PerforceProvider) FormatVCSBranch(branch string) string { return branch } +func (p *PerforceProvider) FormatRevision(rev string) string { return rev } func (p *PerforceProvider) DiffHint(firstRev, lastRev string) string { return fmt.Sprintf("p4 changes -l @%s,@%s", firstRev, lastRev) diff --git a/internal/cmd/vcs_provider.go b/internal/cmd/vcs_provider.go index d20c6e4..56d5c91 100644 --- a/internal/cmd/vcs_provider.go +++ b/internal/cmd/vcs_provider.go @@ -10,7 +10,6 @@ type VCSProvider interface { BranchExistsOnRemote(branch string) bool PushBranch(branch string) error FormatRevision(rev string) string - FormatVCSBranch(branch string) string DiffHint(firstRev, lastRev string) string } diff --git a/internal/cmd/vcs_provider_test.go b/internal/cmd/vcs_provider_test.go index c26f95a..df5b705 100644 --- a/internal/cmd/vcs_provider_test.go +++ b/internal/cmd/vcs_provider_test.go @@ -17,14 +17,6 @@ func TestGitProvider_FormatRevision(t *testing.T) { assert.Equal(t, "abc1234", p.FormatRevision("abc1234")) } -func TestGitProvider_FormatVCSBranch(t *testing.T) { - p := &GitProvider{} - assert.Equal(t, "refs/heads/main", p.FormatVCSBranch("main")) - assert.Equal(t, "refs/heads/feature/test", p.FormatVCSBranch("feature/test")) - assert.Equal(t, "refs/tags/v1.0", p.FormatVCSBranch("refs/tags/v1.0")) - assert.Equal(t, "", p.FormatVCSBranch("")) -} - func TestGitProvider_DiffHint(t *testing.T) { assert.Equal(t, "git diff abc1234^..def1234", (&GitProvider{}).DiffHint("abc1234567890", "def1234567890")) } @@ -98,12 +90,6 @@ func TestPerforceProvider_FormatRevision(t *testing.T) { assert.Equal(t, "1", p.FormatRevision("1")) } -func TestPerforceProvider_FormatVCSBranch(t *testing.T) { - p := &PerforceProvider{} - assert.Equal(t, "//depot/main", p.FormatVCSBranch("//depot/main")) - assert.Equal(t, "//stream/dev", p.FormatVCSBranch("//stream/dev")) -} - func TestPerforceProvider_DiffHint(t *testing.T) { assert.Equal(t, "p4 changes -l @100,@200", (&PerforceProvider{}).DiffHint("100", "200")) }