-
Notifications
You must be signed in to change notification settings - Fork 8
feat: add support for generic Git repository #279
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?
feat: add support for generic Git repository #279
Conversation
|
Thanks @edoardolincetto, reviewing ASAP! |
|
@bfabio I've also drafted some tests. Do you think testing individual utilities is enough, or should we test the entire process with a dedicated test repository? The testing repo should be hosted outside GitHub, GitLab and Bitbucket. |
bfabio
left a comment
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.
Sorry for the delay @edoardolincetto.
git_helper.go
Outdated
| } | ||
|
|
||
| // Initialize sparse checkout | ||
| cmd = exec.Command("git", "sparse-checkout", "init", "--cone") |
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.
This creates a dependency on git and I'd rather not. Let's use a library here so it has a chance to work in WASM as well (if we implement storage for the clone someway)
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.
I've migrated to go-git to remove the dependency on git. Since go-git doesn't currently support partial clones, the new implementation is slower (especially for large repositories) but it should be fine for typical use cases.
| config := publiccode.ParserConfig{BaseURL: *localBasePathPtr} | ||
| config.DisableNetwork = *disableNetworkPtr | ||
| config.DisableExternalChecks = *disableExternalChecksPtr | ||
| config.AllowLocalGitClone = *allowLocalGitClonePtr |
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.
I think at this point it makes sense to refactor those options to be more ergonomic.
--no-network, --no-external-checks and this new --allow-local-git-clone are interconnected and they could be used in contradicting ways, eg. --no-external-checks used together with --allow-local-git-clone.
Let's make a new option --external-checks with an argument.
--external-checks=none--external-checks=local--external-checks=clone
Care to make a PR for the first two, so that we can implement --external-checks=clone afterwards?
| // fileExists returns true if the file resource exists. | ||
| func (p *Parser) fileExists(u url.URL, network bool) (bool, error) { | ||
| // Returns true if the file resource exists. | ||
| func (p *Parser) fileExists(u url.URL, network bool, isGitRepo bool) (bool, error) { |
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.
Not a fan of the signature of this function, but I suppose that with the --external-checks refactor we'd consolidate the two bools into a enum.
Something like `fileExists(url url.URL, checkMode CheckMode).
git_helper.go
Outdated
| } | ||
|
|
||
| // Checks if a file exists in a Git repository by attempting to check it out. | ||
| func (g *gitHelper) fileExistsInRepo(repoURL string, filePath string) (bool, string, error) { |
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.
We should also handle the case where the publiccode.yml is already in a cloned repo.
git_helper.go
Outdated
| // Checks if an URL is a generic Git repository URL. | ||
| // Returns false for supported hosting platforms (GitHub, GitLab, Bitbucket) | ||
| // which have web interfaces and should not use local Git cloning. | ||
| func isGitURL(u *url.URL) bool { |
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.
Commenting on this function, but it's a general remark.
isGitURL, isGitRepo, are misleading. We should pick an alternative way of expressing the concept: They're always git repos in all cases, and it might be confusing to follow the code with this naming.
git_helper.go
Outdated
| return false, "", err | ||
| } | ||
|
|
||
| // Try to checkout the file |
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.
There are a lot of comments like this one. Was this PR AI assisted?
Closes #63
This PR adds support for validating file references (logos, images and authorsFile) in publiccode.yml files for generic Git repositories that are not hosted on supported platforms (GitHub, GitLab and Bitbucket).
--allow-local-git-cloneCLI flag to enable the feature.allowLocalGitCloneParser field to enable the feature.