-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: Enable --skip-clone-no-changes for fork PRs (#3891) #3900
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: Enable --skip-clone-no-changes for fork PRs (#3891) #3900
Conversation
did you ever tried this : https://www.runatlantis.io/docs/server-configuration.html#checkout-depth |
I did not, but I was able to confirm that hasRepoCfg is always false for fork PRs. Since that is the case a clone will always performed, which seems to contradict the docs for Can you elaborate on how |
I thought your concern was the size of the clone and not the amount of git clone invocations |
The size is not an issue if Without this change the call to Since deploying this branch yesterday, many forked PRs have skipped the cloning flow: Logs
|
Hi @jamengual, just bumping this thread in hopes this bug fix can get merged in! |
@Ulminator Can you resolve the file conflict and we'll get this in the next patch release. Thanks |
Head branch was pushed to by a user without write access
a0ca05d
to
5e40641
Compare
@GenPage just letting you know that I fixed the merge conflicts! |
5e40641
to
aa58a11
Compare
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
The name of the commit/branch/tag. Default the repository’s default branch (usually master)
@runatlantis/maintainers I know this has been stagnant for awhile, but I'd really like to get this merged. Since I originally wrote this PR the |
@X-Guardian could you look at the latest updates? Thanks. |
@jamengual thank you for taking a look! @X-Guardian please let me know if there are any further changes that need to be made! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Ulminator, just a small request to restore a debug line.
Can you also update the PR description, as the change doesn't actually check for a forked PR, it just uses the headRepo
references from the context, rather than the baseRepo
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you restore this debug message that you have removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since pull models.PullRequest
is not being passed in anymore I do not have access to pull.Num
, which is why I added this debug log in project_command_builder.go
:
atlantis/server/events/project_command_builder.go
Lines 351 to 352 in 9b981f9
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) |
I did update the log though to just have the repoCfgFile
and the ctx.Pull.Num
, instead of just the full ctx.Pull
struct.
Is this okay with you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logger should already have the repo and pull request context set, so I would recommend leaving the debug message where it is, but just changing it to match the GitLab one, i.e.
logger.Debug("Getting GitHub file content for file '%s'", fileName)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the PR description. Can you also update the PR title to better reflect the change (This is used for the changelog entry when it is released)
Signed-off-by: Matt Ulmer <[email protected]>
Signed-off-by: Matt Ulmer <[email protected]>
Signed-off-by: Matt Ulmer <[email protected]>
Signed-off-by: Matt Ulmer <[email protected]>
9b981f9
to
d437256
Compare
PTAL when possible! Had to resolve some merge conflicts as there were recent changes to |
Hi @Ulminator, can you resolve the conflict in the test file? |
This issue is stale because it has been open for 1 month with no activity. Remove stale label or comment or this will be closed in 1 month. |
Apologies for not resolving the outstanding issues with the PR. Just commenting to prevent the PR from closing as I plan to revisit this within the next few weeks. |
what
This PR modifies the function signature of
VCSClient.GetFileContent
and enforces that when attempting to check therepoCfgFile
of theHeadBranch
, theHeadRepo
is used instead of theBaseRepo
.why
If one is using a forked repo, the
HeadBranch
will not exist on theBaseRepo
(or if it does, it's not the branch you actually want).While working in a monorepo that others would fork, the disk space on the VM that Atlantis runs on quickly filled up as every PR made would be cloned. This should not happen when the
--skip-clone-no-changes
flag is used, but the logic for that currently ignores fork PRs.tests
I have deployed my branch and validated that PRs from forked repos are not cloned if the files changed in them are outside project directories.
make test
passed as well.Our workflow only uses the GitHub client so the implementation of the GitLab client has not been tested.
references