feat: repo health check (#16) #17
No reviewers
Labels
No Label
bug
component
executor
component
framework
component
parser
component
UI
duplicate
enhancement
help wanted
invalid
priority
p0
priority
p1
priority
p2
priority
p3
question
wontfix
No Milestone
No project
No Assignees
8 Participants
Notifications
Due Date
No due date set.
Blocks
#16 repo health executable & parser
JOJ/JOJ3
Reference: JOJ/JOJ3#17
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "file_check"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
You need to make it build as a binary that accepts command line arguments to check the repo by calling your functions in
cmd/healthcheck/main.go
, and also modifiy theMakefile
.Do not use
fmt.Println
to output all kinds of message instdout
. The parser needs to know which part is the output and which part is just logs.You also need to implement the parser for it and append how to use it in README so that we can test it in the complete workflow.
@ -123,3 +123,4 @@ $RECYCLE.BIN/
# Custom rules (everything added below won't be overriden by 'Generate .gitignore File' if you use 'Update' option)
build/
.DS_Store
I've updated it in master branch, this line is repeated now
@ -0,0 +9,4 @@
"strings"
)
func getRoot() (string, error) {
I think the root dir of the git repo should either be cwd or appointed in config
@ -0,0 +1,3 @@
[repo]
the healthcheck binary should use command line arguments as config only
@ -0,0 +59,4 @@
}
if len(nonAscii) > 0 {
panic("Non-ASCII characters found in the following files:\n" + strings.Join(nonAscii, "\n"))
And do not just
panic
on these errors. These errors are not unrecoverable. You can just record these errors and let the program continue working on the other parts. Or just return the error to the upper level and let that function decides what is the next step.@ -3,49 +3,50 @@ module focs.ji.sjtu.edu.cn/git/FOCS-dev/JOJ3
go 1.22.0
I don't think you need to change this file.
@ -52,6 +52,8 @@ github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99 h1:BQSFePA1RWJOl
github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99/go.mod h1:1lJo3i6rXxKeerYnT8Nvf0QmHCRC1n8sfWVwXF2Frvo=
github.com/jinzhu/copier v0.4.0 h1:w3ciUoD19shMCRargcpm0cm91ytaBhDvuRpz1ODO/U8=
github.com/jinzhu/copier v0.4.0/go.mod h1:DfbEm0FYsaqBcKcFuvmOZb218JkPGtvSHsKg8S8hyyg=
github.com/jstemmer/gotags v1.4.1 h1:aWIyXsU3lTDqhsEC49MP85p2cUUWr2ptvdGNqqGA3r4=
ditto
It is wrongly changed
repo health checkto WIP: repo health check@ -0,0 +1,27 @@
package healthcheck
util.go
for consistency@ -0,0 +5,4 @@
"regexp"
)
func addExt(fileList []string, ext string) {
Where is it called?
@ -1 +1,62 @@
package healthcheck
See https://focs.ji.sjtu.edu.cn/git/git/FOCS-dev/JOJ3/pulls/17#issuecomment-296278
The binary to be called in command line needs to be compiled from
cmd/healthcheck
, just likejoj3
is compiled fromcmd/joj3
.it seems that I'm not authorized
FOCS-dev/JOJ3#17 (comment)
@ -2,0 +16,4 @@
return nil
}
func (m *multiStringValue) String() string {
why?
the cli argument doesn't provide slice, so I define functions and types to create an api for that
It would be easier to use char like
,
to separate cli argument of the same flag, but I didn't see this kind of argument in my experience. The usual case should becmd -flag1=test -flag1=test2
? I don't sure which one is correct.@ -0,0 +1,48 @@
package healthcheck
No underscores in filename, check https://google.github.io/styleguide/go/decisions#naming
I feel puzzled about this request. It seems that there is no logging part in origin code https://focs.ji.sjtu.edu.cn/git/TAs/resources/src/branch/drone/dronelib.checks. How will we handle the stdout and log separately?
Check
27a1e4420e
and38d1c18471
.No need to copy the exact behavior of dronelib.checks. It needs to fit for the current executor+parser architecture.
WIP: repo health checkto WIP: feat: repo health check (#16)@ -0,0 +17,4 @@
StdErr: "",
}
cmd := exec.Command("git", "count-objects", "-v")
add a comment about link to https://github.com/go-git/go-git/blob/master/COMPATIBILITY.md#plumbing-commands , we just can not use go-git to implement it.
I think this branch needs to rebase on master and use
make test
for testing.d6d5bf0f2d
to7b8e3902ba
@ -18,3 +18,31 @@
path = examples/dummy/error
url = ssh://git@focs.ji.sjtu.edu.cn:2222/FOCS-dev/JOJ3-examples.git
branch = dummy/error
[submodule "/home/zzjc/joj/JOJ3/examples/repohealth/largefile"]
local path
This PR is grown larger than expected, make it hard to review. Could you please split each feature to a separated PR and only add corresponding tests in each commit?
So basically we should create a branch for each feature and create a pr to
main
?Or with current testing setup, you can continue working on this branch and write a test case for each situation (basically pass and fail(s) for each check). I can merge it if all the test cases are passed.
OK, I got it.
@ -0,0 +21,4 @@
StdErr: "",
}
cmd := exec.Command("git", "log", "--encoding=UTF-8", "--format=%B")
https://github.com/go-git/go-git#in-memory-example
Try to avoid
exec.Command
.@ -0,0 +17,4 @@
StdErr: "",
}
cmd := exec.Command("git", "count-objects", "-v")
@manuel :
add a "TODO comment" in the source code with
The ref to submodule
keyword/clang-tidy/sillycode
crashed?BTW how should we fix issue caused by
sandbox
in copying the git repo? Can we simply skip the test related to git and test it manually? Also, I'm wondering how we deploy it on drone for real usage.Oh sorry I renamed it, it's now
keyword/clangtidy/sillycode
, please make a new referenceokk
Most of test case is ready except for release and repoverify. I think maybe we can test it in production?
@ -0,0 +1 @@
{healthcheck [{0 [{"name of check":"RepoSize","stdout":"Checking repository size: OK","exit code":0,"stderr":"","errorLog":null},{"name of check":"forbiddenFile","stdout":"Checking forbidden files: Failed","exit code":103,"stderr":"103the following forbidden files were found: conf.toml, healthcheck, stderr, stdout, \n\nTo fix it, first make a backup of your repository and then run the following commands:\nfor i in conf.toml healthcheck stderr stdout ; do git filter-repo --force --invert-paths --path \\\"\\$i\\\"; done\ngit remote add origin \ngit push --set-upstream origin --force","errorLog":null},{"name of check":"metaFile","stdout":"Checking the existence of meta file: OK","exit code":0,"stderr":"","errorLog":null},{"name of check":"NonAsciiFiles","stdout":"Checking for non-ascii files: Failed","exit code":105,"stderr":"Non-ASCII characters found in the following files:\n\tnonascii.txt","errorLog":null},{"name of check":"NonAsciiMsg","stdout":"Checking for non-ASCII characters in commit message: OK","exit code":0,"stderr":"","errorLog":null}]}]}
remove
@ -0,0 +1 @@
{healthcheck [{0 [{"name of check":"RepoSize","stdout":"Checking repository size: OK","exit code":0,"stderr":"","errorLog":null},{"name of check":"forbiddenFile","stdout":"Checking forbidden files: Failed","exit code":103,"stderr":"103the following forbidden files were found: conf.toml, healthcheck, stderr, stdout, \n\nTo fix it, first make a backup of your repository and then run the following commands:\nfor i in conf.toml healthcheck stderr stdout ; do git filter-repo --force --invert-paths --path \\"\$i\\"; done\ngit remote add origin \ngit push --set-upstream origin --force","errorLog":null},{"name of check":"metaFile","stdout":"Checking the existence of meta file: OK","exit code":0,"stderr":"","errorLog":null},{"name of check":"NonAsciiFiles","stdout":"Checking for non-ascii files: Failed","exit code":105,"stderr":"Non-ASCII characters found in the following files:\n\tnonascii.txt","errorLog":null},{"name of check":"NonAsciiMsg","stdout":"Checking for non-ASCII characters in commit message: OK","exit code":0,"stderr":"","errorLog":null}]}]}
remove
@ -0,0 +61,4 @@
tmp = healthcheck.NonAsciiMsg(*rootDir)
info = append(info, tmp)
// TODO: find a way to test the release tag
is it still TODO?
Done yet
@ -0,0 +30,4 @@
}
return stage.ParserResult{
Score: 0,
Comment: stdout,
Comments need to be human-readable. You can use markdown format here. I suggest only keep the stderr field. You can get exitcode directly from executor if needed. Use logger to log any details for student to debug as they can check the log info from drone.
A comment of health check should look like this:
Repo Size
Pass
Forbidden File
The following forbidden files were found: README.md, conf.toml, expected.json, healthcheck, stderr, stdin.sh, stdout
To fix it, first make a backup of your repository and then run the following commands:
Check name...
check detail...
5a4c4a40b7
toa8c5eb7c17
a8c5eb7c17
to1b84febad8
9e195f00ad
toab0c7b1429
d7fd1f1786
tofd643e16eb
fd643e16eb
tofce3031ed0
Unit test updated and passed
Do we need to adjust the forbidden file check before merging?
Quickly check how long it would take to do it. If quick, then we can include it now, otherwise we can merge now and see later for that extra feature.
need more info on how this would work?
I think it should be easy. I think I can can write a small function in forbidden file check and enable it with cli args so that it can fulfill this feature while don't disturbing the requirement of the other courses.
But I am thinking about how to enable our code to support more features just as a plugin. I think it might be helpful for future maintenance.
at the moment in the config file we have a
GITLOCALWHITELIST
variable storing a filename. then this file can contain one dirname per line.if
GITLOCALWHITELIST
is not set in the config file for the repo then it's as if there was no such file (teaching team decides whether or not there is a need for such file). students can add their file but it will be ignored for JOJ... so probably no need for a command line argument. either the file is allowed (defined in toml file) and exists, then we consider it or we ignore it (we don't even know about it)in the loop to detect all forbidden files i simply added this line. it ignores/continue if the dir is in whitelisted
i don't see any need to allow filenames or patterns in this file as this is only to allow free directory structure.
@ -0,0 +56,4 @@
}
}
if !found {
return fmt.Errorf("Wrong release tag '%s'. Please use one of '%s'.", target, strings.Join(tags, "', '"))
Why is the
target
release tag wrong?I intended to mean release tag is missing or wrong.
Do we need to specify missing or wrong? I think only we need to inform students that their release is wrong.
Do we need to test if they submit extra tags?
I think extra is fine, as long as they don't miss any tags.
Then just print the error on missing tags.
c5a2c27126
to960259f037
e620d22e57
tofc01426a9b
@ -0,0 +72,4 @@
if err != nil {
fmt.Printf("## Non-ASCII Characters Commit Message Check Failed:\n%s\n", err.Error())
}
// TODO: find a way to test the release tag
check releases need to use Gitea API, just check tags is enough
Yep, only check tags
Where is it tested?
@ -0,0 +77,4 @@
if err != nil {
fmt.Printf("## Release Tag Check Failed:\n%s\n", err.Error())
}
// FIXME: for drone usage
what should be fixed?
I think it is due to we can only test it when it is deployed on drone so I just skip it when running the code. If not I couldn't push the code.
I think checking an empty value for skipping it is enough. No dir specified = no verify.
@ -0,0 +31,4 @@
return tags, nil
}
func CheckReleases(repoPath string, category string, n int) error {
why not just let the parameter fully determine the target tag? e.g.
func CheckReleases(repoPath, target string)
Because we cannot decide how many hw or project release need to be checked yes. The int n is used to specif thr number
So since we need to pass
category
andn
to this function, we still need to decide the count of hw&proj when generatingconf.toml
. I do not see much difference.Should we change the pattern of the forbidden file check? The argument starts go dirty now. I think we can create a whitelist file in specified folder and pass its path through argument so that it would be easier to configure possibly.
No. We do not need to modify all these arguments manually later. So just keep it dirty and simple here, we will use a conf file generator for this.
@ -115,0 +115,4 @@
### HealthCheck
The repohealth check will return a json list to for check result. The structure of json file is in `pkg/healthcheck/util.go`
It is outdated now.
@ -0,0 +37,4 @@
}
if actualChecksum == expectedChecksum {
return true, "" // fmt.Sprintf("Checksum for %s passed!", filePath)
remove the comment
@ -0,0 +44,4 @@
}
func VerifyFiles(rootDir string, checkFileNameList string, checkFileSumList string) error {
// Parse command-line arguments
ditto
@ -0,0 +52,4 @@
// Process input file names and checksums
if len(checkFileNameList) == 0 {
return nil // fmt.Errorf("No checksum happened")
os.Exit(1)
remove it
@ -0,0 +61,4 @@
if len(fileNames) != len(checkSums) {
return fmt.Errorf("Error: The number of files and checksums do not match.")
os.Exit(1)
remove it
I mean remove the
os.Exit(1)
, and also the one on line 55.@ -0,0 +80,4 @@
}
}
if allPassed {
return nil // fmt.Errorf("Congratulations! All checksums passed!")
ditto
@ -0,0 +82,4 @@
if allPassed {
return nil // fmt.Errorf("Congratulations! All checksums passed!")
} else {
return fmt.Errorf("Some checksums failed. Please review the errors below:")
Why so many returns?
@ -0,0 +66,4 @@
errorMessages = append(errorMessages, message)
}
}
if allPassed {
why not just return error on not all passed?
79e42c366e
to102b0ef2a5
WIP: feat: repo health check (#16)to feat: repo health check (#16)merged??? I see many unresolved comments...