Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Enable --skip-clone-no-changes for fork PRs (#3891) #3900

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix: Enable --skip-clone-no-changes for fork PRs (#3891) #3900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cc5e77c
fix: Enable --skip-clone-no-changes for fork PRs (#3891)
Ulminator Oct 24, 2023
1fb9cd5
Merge branch 'main' into skip_clone_no_changes_fork_prs
X-Guardian Nov 8, 2024
d209151
fix: add test case for forked scenario
Ulminator Nov 9, 2024
d61c9b7
fix: simplify logic
Ulminator Nov 9, 2024
b01c67b
Merge branch 'main' into skip_clone_no_changes_fork_prs
Ulminator Nov 18, 2024
56c3327
Merge branch 'main' into skip_clone_no_changes_fork_prs
Ulminator Dec 3, 2024
b0def7d
Merge branch 'main' into skip_clone_no_changes_fork_prs
Ulminator Dec 16, 2024
711f614
Merge branch 'main' into skip_clone_no_changes_fork_prs
jamengual Dec 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion server/events/project_command_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,10 @@ func (p *DefaultProjectCommandBuilder) buildAllCommandsByCfg(ctx *command.Contex

if p.SkipCloneNoChanges && p.VCSClient.SupportsSingleFileDownload(ctx.Pull.BaseRepo) {
repoCfgFile := p.GlobalCfg.RepoConfigFile(ctx.Pull.BaseRepo.ID())
hasRepoCfg, repoCfgData, err := p.VCSClient.GetFileContent(ctx.Log, ctx.Pull, repoCfgFile)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for this new code?

Copy link
Author

@Ulminator Ulminator Nov 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I just added a test case, regenerated the mock_client.go file with pegomock, and fixed the issue w/ the DCO status check on my first commit.

The test also made it clear I could simplify some of the logic with just always passing the HeadRepo to GetFileContent. The prior issue being the repoCfgFile never being found when attempting to get it from the HeadBranch on the BaseRepo, but that branch not existing as it is just on the HeadRepo in the case of a forked PR.

ctx.Log.Debug("Getting file content for pull request %+v", ctx.Pull)
hasRepoCfg, repoCfgData, err := p.VCSClient.GetFileContent(ctx.Log, ctx.HeadRepo, ctx.Pull.HeadBranch, repoCfgFile)

if err != nil {
return nil, errors.Wrapf(err, "downloading %s", repoCfgFile)
}
Expand Down
35 changes: 29 additions & 6 deletions server/events/project_command_builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1624,6 +1624,7 @@ projects:
func TestDefaultProjectCommandBuilder_SkipCloneNoChanges(t *testing.T) {
cases := []struct {
AtlantisYAML string
IsFork bool
ExpectedCtxs int
ExpectedClones InvocationCountMatcher
ModifiedFiles []string
Expand All @@ -1640,6 +1641,16 @@ projects:
{
AtlantisYAML: `
version: 3
projects:
- dir: dir1`,
IsFork: true,
ExpectedCtxs: 0,
ExpectedClones: Never(),
ModifiedFiles: []string{"dir2/main.tf"},
},
{
AtlantisYAML: `
version: 3
parallel_plan: true`,
ExpectedCtxs: 0,
ExpectedClones: Once(),
Expand Down Expand Up @@ -1668,7 +1679,7 @@ projects:
Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(c.ModifiedFiles, nil)
When(vcsClient.SupportsSingleFileDownload(Any[models.Repo]())).ThenReturn(true)
When(vcsClient.GetFileContent(
Any[logging.SimpleLogging](), Any[models.PullRequest](), Any[string]())).ThenReturn(true, []byte(c.AtlantisYAML), nil)
Any[logging.SimpleLogging](), Any[models.Repo](), Any[string](), Any[string]())).ThenReturn(true, []byte(c.AtlantisYAML), nil)
workingDir := mocks.NewMockWorkingDir()

logger := logging.NewNoopLogger(t)
Expand Down Expand Up @@ -1706,20 +1717,32 @@ projects:

var actCtxs []command.ProjectContext
var err error

baseRepo := models.Repo{Owner: "owner"}
headRepo := baseRepo
if c.IsFork {
headRepo.Owner = "repoForker"
}

actCtxs, err = builder.BuildAutoplanCommands(&command.Context{
HeadRepo: models.Repo{},
Pull: models.PullRequest{},
User: models.User{},
Log: logger,
Scope: scope,
HeadRepo: headRepo,
Pull: models.PullRequest{
BaseRepo: baseRepo,
},
User: models.User{},
Log: logger,
Scope: scope,
PullRequestStatus: models.PullReqStatus{
Mergeable: true,
},
})

Ok(t, err)
Equals(t, c.ExpectedCtxs, len(actCtxs))
workingDir.VerifyWasCalled(c.ExpectedClones).Clone(Any[logging.SimpleLogging](), Any[models.Repo](),
Any[models.PullRequest](), Any[string]())
_, actRepo, _, _ := vcsClient.VerifyWasCalled(Once()).GetFileContent(Any[logging.SimpleLogging](), Any[models.Repo](), Any[string](), Any[string]()).GetCapturedArguments()
Equals(t, headRepo, actRepo)
}
}

Expand Down
2 changes: 1 addition & 1 deletion server/events/vcs/azuredevops_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@ func (g *AzureDevopsClient) SupportsSingleFileDownload(repo models.Repo) bool {
return false
}

func (g *AzureDevopsClient) GetFileContent(_ logging.SimpleLogging, pull models.PullRequest, fileName string) (bool, []byte, error) { //nolint: revive
func (g *AzureDevopsClient) GetFileContent(_ logging.SimpleLogging, _ models.Repo, _ string, _ string) (bool, []byte, error) { //nolint: revive
return false, []byte{}, fmt.Errorf("not implemented")
}

Expand Down
2 changes: 1 addition & 1 deletion server/events/vcs/bitbucketcloud/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ func (b *Client) SupportsSingleFileDownload(models.Repo) bool {
// GetFileContent a repository file content from VCS (which support fetch a single file from repository)
// The first return value indicates whether the repo contains a file or not
// if BaseRepo had a file, its content will placed on the second return value
func (b *Client) GetFileContent(_ logging.SimpleLogging, _ models.PullRequest, _ string) (bool, []byte, error) {
func (b *Client) GetFileContent(_ logging.SimpleLogging, _ models.Repo, _ string, _ string) (bool, []byte, error) {
return false, []byte{}, fmt.Errorf("not implemented")
}

Expand Down
2 changes: 1 addition & 1 deletion server/events/vcs/bitbucketserver/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ func (b *Client) SupportsSingleFileDownload(_ models.Repo) bool {
// GetFileContent a repository file content from VCS (which support fetch a single file from repository)
// The first return value indicates whether the repo contains a file or not
// if BaseRepo had a file, its content will placed on the second return value
func (b *Client) GetFileContent(_ logging.SimpleLogging, _ models.PullRequest, _ string) (bool, []byte, error) {
func (b *Client) GetFileContent(_ logging.SimpleLogging, _ models.Repo, _ string, _ string) (bool, []byte, error) {
return false, []byte{}, fmt.Errorf("not implemented")
}

Expand Down
2 changes: 1 addition & 1 deletion server/events/vcs/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ type Client interface {
// GetFileContent a repository file content from VCS (which support fetch a single file from repository)
// The first return value indicates whether the repo contains a file or not
// if BaseRepo had a file, its content will placed on the second return value
GetFileContent(logger logging.SimpleLogging, pull models.PullRequest, fileName string) (bool, []byte, error)
GetFileContent(logger logging.SimpleLogging, repo models.Repo, branch string, fileName string) (bool, []byte, error)
SupportsSingleFileDownload(repo models.Repo) bool
GetCloneURL(logger logging.SimpleLogging, VCSHostType models.VCSHostType, repo string) (string, error)

Expand Down
8 changes: 3 additions & 5 deletions server/events/vcs/gitea/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,13 +421,11 @@ func (c *GiteaClient) GetTeamNamesForUser(repo models.Repo, user models.User) ([
// GetFileContent a repository file content from VCS (which support fetch a single file from repository)
// The first return value indicates whether the repo contains a file or not
// if BaseRepo had a file, its content will placed on the second return value
func (c *GiteaClient) GetFileContent(logger logging.SimpleLogging, pull models.PullRequest, fileName string) (bool, []byte, error) {
logger.Debug("Getting file content for %s in Gitea pull request %d", fileName, pull.Num)

content, resp, err := c.giteaClient.GetContents(pull.BaseRepo.Owner, pull.BaseRepo.Name, pull.HeadCommit, fileName)
Copy link
Author

Choose a reason for hiding this comment

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

With this change removing the models.PullRequest from the interface I opted to changing the head commit here to the branch name which seems to be supported by the docs.

https://pkg.go.dev/code.gitea.io/sdk/gitea#Client.GetContents

https://gitea.com/gitea/go-sdk/src/commit/8b855b4a5b33bc2050143edc4044ee330bf09b05/gitea/repo_file.go#L196

The name of the commit/branch/tag. Default the repository’s default branch (usually master)

func (c *GiteaClient) GetFileContent(logger logging.SimpleLogging, repo models.Repo, branch string, fileName string) (bool, []byte, error) {
content, resp, err := c.giteaClient.GetContents(repo.Owner, repo.Name, branch, fileName)

if err != nil {
logger.Debug("GET /repos/%v/%v/contents/%s?ref=%v returned: %v", pull.BaseRepo.Owner, pull.BaseRepo.Name, fileName, pull.HeadCommit, resp.StatusCode)
logger.Debug("GET /repos/%v/%v/contents/%s?ref=%v returned: %v", repo.Owner, repo.Name, fileName, branch, resp.StatusCode)
return false, nil, err
}

Expand Down
10 changes: 5 additions & 5 deletions server/events/vcs/github_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1072,12 +1072,12 @@ func (g *GithubClient) ExchangeCode(logger logging.SimpleLogging, code string) (
// GetFileContent a repository file content from VCS (which support fetch a single file from repository)
// The first return value indicates whether the repo contains a file or not
// if BaseRepo had a file, its content will placed on the second return value
func (g *GithubClient) GetFileContent(logger logging.SimpleLogging, pull models.PullRequest, fileName string) (bool, []byte, error) {
logger.Debug("Getting file content for %s in GitHub pull request %d", fileName, pull.Num)
opt := github.RepositoryContentGetOptions{Ref: pull.HeadBranch}
fileContent, _, resp, err := g.client.Repositories.GetContents(g.ctx, pull.BaseRepo.Owner, pull.BaseRepo.Name, fileName, &opt)
func (g *GithubClient) GetFileContent(logger logging.SimpleLogging, repo models.Repo, branch string, fileName string) (bool, []byte, error) {
opt := github.RepositoryContentGetOptions{Ref: branch}

fileContent, _, resp, err := g.client.Repositories.GetContents(g.ctx, repo.Owner, repo.Name, fileName, &opt)
if resp != nil {
logger.Debug("GET /repos/%v/%v/contents/%s returned: %v", pull.BaseRepo.Owner, pull.BaseRepo.Name, fileName, resp.StatusCode)
logger.Debug("GET /repos/%v/%v/contents/%s returned: %v", repo.Owner, repo.Name, fileName, resp.StatusCode)
}

if resp.StatusCode == http.StatusNotFound {
Expand Down
8 changes: 4 additions & 4 deletions server/events/vcs/gitlab_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,13 +628,13 @@ func (g *GitlabClient) GetTeamNamesForUser(_ models.Repo, _ models.User) ([]stri
// GetFileContent a repository file content from VCS (which support fetch a single file from repository)
// The first return value indicates whether the repo contains a file or not
// if BaseRepo had a file, its content will placed on the second return value
func (g *GitlabClient) GetFileContent(logger logging.SimpleLogging, pull models.PullRequest, fileName string) (bool, []byte, error) {
func (g *GitlabClient) GetFileContent(logger logging.SimpleLogging, repo models.Repo, branch string, fileName string) (bool, []byte, error) {
logger.Debug("Getting GitLab file content for file '%s'", fileName)
opt := gitlab.GetRawFileOptions{Ref: gitlab.Ptr(pull.HeadBranch)}
opt := gitlab.GetRawFileOptions{Ref: gitlab.Ptr(branch)}

bytes, resp, err := g.Client.RepositoryFiles.GetRawFile(pull.BaseRepo.FullName, fileName, &opt)
bytes, resp, err := g.Client.RepositoryFiles.GetRawFile(repo.FullName, fileName, &opt)
if resp != nil {
logger.Debug("GET /projects/%s/repository/files/%s/raw returned: %d", pull.BaseRepo.FullName, fileName, resp.StatusCode)
logger.Debug("GET /projects/%s/repository/files/%s/raw returned: %d", repo.FullName, fileName, resp.StatusCode)
}
if resp != nil && resp.StatusCode == http.StatusNotFound {
return false, []byte{}, nil
Expand Down
26 changes: 16 additions & 10 deletions server/events/vcs/mocks/mock_client.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion server/events/vcs/not_configured_vcs_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func (a *NotConfiguredVCSClient) SupportsSingleFileDownload(_ models.Repo) bool
return false
}

func (a *NotConfiguredVCSClient) GetFileContent(_ logging.SimpleLogging, _ models.PullRequest, _ string) (bool, []byte, error) {
func (a *NotConfiguredVCSClient) GetFileContent(_ logging.SimpleLogging, _ models.Repo, _ string, _ string) (bool, []byte, error) {
return true, []byte{}, a.err()
}
func (a *NotConfiguredVCSClient) GetCloneURL(_ logging.SimpleLogging, _ models.VCSHostType, _ string) (string, error) {
Expand Down
4 changes: 2 additions & 2 deletions server/events/vcs/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ func (d *ClientProxy) GetTeamNamesForUser(repo models.Repo, user models.User) ([
return d.clients[repo.VCSHost.Type].GetTeamNamesForUser(repo, user)
}

func (d *ClientProxy) GetFileContent(logger logging.SimpleLogging, pull models.PullRequest, fileName string) (bool, []byte, error) {
return d.clients[pull.BaseRepo.VCSHost.Type].GetFileContent(logger, pull, fileName)
func (d *ClientProxy) GetFileContent(logger logging.SimpleLogging, repo models.Repo, branch string, fileName string) (bool, []byte, error) {
return d.clients[repo.VCSHost.Type].GetFileContent(logger, repo, branch, fileName)
}

func (d *ClientProxy) SupportsSingleFileDownload(repo models.Repo) bool {
Expand Down
Loading