feat: Add a 'pull submodules' option to Repositories#3757
feat: Add a 'pull submodules' option to Repositories#3757stith wants to merge 3 commits intosemaphoreui:developfrom
Conversation
utafrali
left a comment
There was a problem hiding this comment.
The feature is well-scoped and covers both git client implementations (CmdGitClient and GoGitClient) consistently. The main issues are: the migration column type (int vs boolean) and MySQL-specific backtick syntax that may break PostgreSQL support, duplicated code in CmdGitClient.Clone, and a subtle Go zero-value default that could silently disable submodules when the struct is constructed outside the frontend path.
| @@ -0,0 +1 @@ | |||
| alter table `project__repository` add `pull_submodules` boolean not null DEFAULT true; | |||
There was a problem hiding this comment.
Two issues here:
- The column type is
intbut the Go struct field isbool. Other parts of the codebase useboolean(ortinyint(1)) for boolean columns, which maps more naturally to Go'sbool. Change to:
alter table `project__repository` add `pull_submodules` boolean not null default true;- The backtick syntax is MySQL-specific. Since
db/Migration.goaccepts adialectparameter, the project likely supports multiple databases (e.g. PostgreSQL). Check whether this migration needs a PostgreSQL variant, or whether the project has a mechanism to handle dialect differences for migrations.
| dirName = r.TmpDirName | ||
| } | ||
|
|
||
| if r.Repository.PullSubmodules { |
There was a problem hiding this comment.
The Clone function now has two nearly identical c.run(...) calls that diverge only by the presence of --recursive. This is harder to maintain and easy to accidentally desync. The GoGitClient approach is better -- build the args list conditionally before calling run:
args := []string{"clone", "--branch", r.Repository.GitBranch, r.Repository.GetGitURL(false), dirName}
if r.Repository.PullSubmodules {
args = append([]string{"clone", "--recursive", "--branch", r.Repository.GitBranch, r.Repository.GetGitURL(false), dirName}, []string{}...)
// or build args slice and insert --recursive at position 1
}
return c.run(r, GitRepositoryTmpPath, args...)A cleaner approach:
cloneArgs := []string{"clone"}
if r.Repository.PullSubmodules {
cloneArgs = append(cloneArgs, "--recursive")
}
cloneArgs = append(cloneArgs, "--branch", r.Repository.GitBranch, r.Repository.GetGitURL(false), dirName)
return c.run(r, GitRepositoryTmpPath, cloneArgs...)|
|
||
| // Pull the latest changes from the origin remote and merge into the current branch | ||
| err = wt.Pull(&git.PullOptions{RemoteName: "origin", | ||
| pullOpts := &git.PullOptions{ |
There was a problem hiding this comment.
Minor redundancy: RecurseSubmodules is set to git.DefaultSubmoduleRecursionDepth in the struct literal, then immediately overridden to git.NoRecurseSubmodules if !PullSubmodules. It reads more clearly to initialize it to git.NoRecurseSubmodules and only set the recursive depth when needed -- matching the pattern used in the Clone method above:
pullOpts := &git.PullOptions{
RemoteName: "origin",
Auth: authMethod,
RecurseSubmodules: git.NoRecurseSubmodules,
}
if r.Repository.PullSubmodules {
pullOpts.RecurseSubmodules = git.DefaultSubmoduleRecursionDepth
}| ProjectID int `db:"project_id" json:"project_id" backup:"-"` | ||
| GitURL string `db:"git_url" json:"git_url" binding:"required"` | ||
| GitBranch string `db:"git_branch" json:"git_branch" binding:"required"` | ||
| SSHKeyID int `db:"ssh_key_id" json:"ssh_key_id" binding:"required" backup:"-"` |
There was a problem hiding this comment.
The zero value of bool in Go is false, so any code path that constructs a Repository struct without explicitly setting PullSubmodules (e.g. direct API calls, backup restore, or internal tooling) will silently disable submodule pulling. The database default of 1 only applies to rows inserted without that column, not to the application layer.
Consider whether the API handler that reads the request body into a Repository struct needs to default this field to true when it is absent from the JSON payload. A JSON omitempty tag would also prevent the frontend from explicitly sending false when unchecked. At minimum, this edge case is worth a comment.
|
is this the official review bot for this project? it doesn't seem to have any past activity. what's up with that? |
|
Hi @stith no, it is not official review bot. |
|
what's the process for review on this? do i need to open an issue for discussion first? |
Our Semaphore instance uses a monorepo as its Ansible playbooks source, and that monorepo has several large submodules that we don't care about when doing Ansible runs, which adds ~5 minutes to each new clone. This PR adds a "Pull submodules" option (defaulting to
trueto match current behavior) which lets us turn that behavior off.This is my first time contributing to this project and I'm not sure how to decide which version number to use for the migrations, so advice is welcome!