feat: repo health check (#16) #17

Merged
张泊明518370910136 merged 37 commits from file_check into master 2024-09-11 20:09:27 +08:00
  • repo size
  • forbidden files
  • meta files
  • ascii character in files
  • integrity check
  • ascii character in the commit message
  • release tag check
- repo size - forbidden files - meta files - ascii character in files - integrity check - ascii character in the commit message - release tag check
manuel was assigned by 周赵嘉程521432910016 2024-03-11 08:49:33 +08:00
张泊明518370910136 was assigned by 周赵嘉程521432910016 2024-03-11 08:49:33 +08:00
焦天勤522370910029 was assigned by 周赵嘉程521432910016 2024-03-11 08:49:53 +08:00
张泊明518370910136 requested changes 2024-03-11 09:51:13 +08:00
张泊明518370910136 left a comment
Owner

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 the Makefile.

Do not use fmt.Println to output all kinds of message in stdout. 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.

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 the `Makefile`. Do not use `fmt.Println` to output all kinds of message in `stdout`. 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.
.gitignore Outdated
@ -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

I've updated it in master branch, this line is repeated now
zzjc123 marked this conversation as resolved
@ -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

I think the root dir of the git repo should either be cwd or appointed in config
zzjc123 marked this conversation as resolved
@ -0,0 +1,3 @@
[repo]

the healthcheck binary should use command line arguments as config only

the healthcheck binary should use command line arguments as config only
zzjc123 marked this conversation as resolved
张泊明518370910136 reviewed 2024-03-11 09:55:39 +08:00
@ -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.

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.
zzjc123 marked this conversation as resolved
张泊明518370910136 requested changes 2024-03-12 04:14:24 +08:00
go.mod Outdated
@ -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.

I don't think you need to change this file.
zzjc123 marked this conversation as resolved
go.sum Outdated
@ -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

ditto
Author
Member

It is wrongly changed

It is wrongly changed
zzjc123 marked this conversation as resolved
张泊明518370910136 changed title from repo health check to WIP: repo health check 2024-03-12 04:14:28 +08:00
张泊明518370910136 requested changes 2024-03-12 15:28:41 +08:00
@ -0,0 +1,27 @@
package healthcheck

util.go for consistency

`util.go` for consistency
zzjc123 marked this conversation as resolved
@ -0,0 +5,4 @@
"regexp"
)
func addExt(fileList []string, ext string) {

Where is it called?

Where is it called?
zzjc123 marked this conversation as resolved
@ -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 like joj3 is compiled from cmd/joj3.

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 like `joj3` is compiled from `cmd/joj3`.
Author
Member

it seems that I'm not authorized

it seems that I'm not authorized
https://focs.ji.sjtu.edu.cn/git/FOCS-dev/JOJ3/pulls/17#issuecomment-296316
zzjc123 marked this conversation as resolved
@ -2,0 +16,4 @@
return nil
}
func (m *multiStringValue) String() string {

why?

why?
Author
Member

the cli argument doesn't provide slice, so I define functions and types to create an api for that

the cli argument doesn't provide slice, so I define functions and types to create an api for that
Author
Member

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 be cmd -flag1=test -flag1=test2? I don't sure which one is correct.

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 be `cmd -flag1=test -flag1=test2`? I don't sure which one is correct.
bomingzh marked this conversation as resolved
张泊明518370910136 reviewed 2024-03-12 15:30:29 +08:00
@ -0,0 +1,48 @@
package healthcheck
No underscores in filename, check https://google.github.io/styleguide/go/decisions#naming
zzjc123 marked this conversation as resolved
Author
Member

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 the Makefile.

Do not use fmt.Println to output all kinds of message in stdout. 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.

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?

> 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 the `Makefile`. > > Do not use `fmt.Println` to output all kinds of message in `stdout`. 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. 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?

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 the Makefile.

Do not use fmt.Println to output all kinds of message in stdout. 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.

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 and 38d1c18471.

No need to copy the exact behavior of dronelib.checks. It needs to fit for the current executor+parser architecture.

> > 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 the `Makefile`. > > > > Do not use `fmt.Println` to output all kinds of message in `stdout`. 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. > > 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 https://focs.ji.sjtu.edu.cn/git/FOCS-dev/JOJ3/commit/27a1e4420e1365f0ee268384a164ace0853fe4d6 and https://focs.ji.sjtu.edu.cn/git/FOCS-dev/JOJ3/commit/38d1c18471a649274a2157e918dd21472fdc0554. No need to copy the exact behavior of dronelib.checks. It needs to fit for the current executor+parser architecture.
张泊明518370910136 changed title from WIP: repo health check to WIP: feat: repo health check (#16) 2024-03-13 04:40:13 +08:00
周赵嘉程521432910016 requested review from 张泊明518370910136 2024-03-21 12:33:06 +08:00
张泊明518370910136 reviewed 2024-03-22 14:24:42 +08:00
@ -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.

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.
bomingzh marked this conversation as resolved
张泊明518370910136 added a new dependency 2024-03-23 03:32:28 +08:00

I think this branch needs to rebase on master and use make test for testing.

I think this branch needs to rebase on master and use `make test` for testing.
张泊明518370910136 force-pushed file_check from d6d5bf0f2d to 7b8e3902ba 2024-03-31 14:18:12 +08:00 Compare
张泊明518370910136 reviewed 2024-04-09 01:10:56 +08:00
.gitmodules Outdated
@ -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

local path
bomingzh marked this conversation as resolved

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?

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?
Author
Member

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?

> 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`?

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.

> > 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.
Author
Member

OK, I got it.

OK, I got it.
张泊明518370910136 reviewed 2024-05-03 10:01:41 +08:00
@ -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`.
zzjc123 marked this conversation as resolved
张泊明518370910136 reviewed 2024-05-03 10:44:32 +08:00
@ -0,0 +17,4 @@
StdErr: "",
}
cmd := exec.Command("git", "count-objects", "-v")

@manuel :
add a "TODO comment" in the source code with

  • a note to reimplement with go-git when available
  • link to feature/compatibility page of go-git
@manuel : add a "TODO comment" in the source code with - a note to reimplement with go-git when available - link to feature/compatibility page of go-git
zzjc123 marked this conversation as resolved
Author
Member

The ref to submodule keyword/clang-tidy/sillycode crashed?

The [ref to submodule](https://focs.ji.sjtu.edu.cn:2222/FOCS-dev/JOJ3/275/1/4) `keyword/clang-tidy/sillycode` crashed?
Author
Member

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.

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.

The ref to submodule keyword/clang-tidy/sillycode crashed?

Oh sorry I renamed it, it's now keyword/clangtidy/sillycode, please make a new reference

> The [ref to submodule](https://focs.ji.sjtu.edu.cn:2222/FOCS-dev/JOJ3/275/1/4) `keyword/clang-tidy/sillycode` crashed? Oh sorry I renamed it, it's now `keyword/clangtidy/sillycode`, please make a new reference
Author
Member

okk

okk
Author
Member

Most of test case is ready except for release and repoverify. I think maybe we can test it in production?

Most of test case is ready except for release and repoverify. I think maybe we can test it in production?
张泊明518370910136 requested changes 2024-05-30 03:45:01 +08:00
1 Outdated
@ -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

remove
zzjc123 marked this conversation as resolved
2 Outdated
@ -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

remove
zzjc123 marked this conversation as resolved
@ -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?

is it still TODO?
Author
Member

Done yet

Done yet
bomingzh marked this conversation as resolved
@ -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...

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...
zzjc123 marked this conversation as resolved
张泊明518370910136 force-pushed file_check from 5a4c4a40b7 to a8c5eb7c17 2024-06-08 15:06:31 +08:00 Compare
张泊明518370910136 force-pushed file_check from a8c5eb7c17 to 1b84febad8 2024-06-08 15:11:39 +08:00 Compare
张泊明518370910136 force-pushed file_check from 9e195f00ad to ab0c7b1429 2024-06-10 06:55:02 +08:00 Compare
张泊明518370910136 force-pushed file_check from d7fd1f1786 to fd643e16eb 2024-06-15 16:06:43 +08:00 Compare
张泊明518370910136 force-pushed file_check from fd643e16eb to fce3031ed0 2024-06-15 16:29:29 +08:00 Compare
Author
Member

Unit test updated and passed

Unit test updated and passed
Author
Member

Do we need to adjust the forbidden file check before merging?

Do we need to adjust the forbidden file check before merging?
Owner

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?

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?
Author
Member

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.

> 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.
Owner

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

	[ -n "$GITLOCALWHITELIST" ] && grep -q $(basename ${forbidden[i]}) $GITLOCALWHITELIST && continue

i don't see any need to allow filenames or patterns in this file as this is only to allow free directory structure.

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 ```sh [ -n "$GITLOCALWHITELIST" ] && grep -q $(basename ${forbidden[i]}) $GITLOCALWHITELIST && continue ``` i don't see any need to allow filenames or patterns in this file as this is only to allow free directory structure.
张泊明518370910136 reviewed 2024-06-19 15:01:45 +08:00
@ -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?

Why is the `target` release tag wrong?

I intended to mean release tag is missing or 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 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?

Do we need to test if they submit extra tags?

I think extra is fine, as long as they don't miss any tags.

I think extra is fine, as long as they don't miss any tags.

Then just print the error on missing tags.

Then just print the error on missing tags.
张泊明518370910136 force-pushed file_check from c5a2c27126 to 960259f037 2024-06-19 15:33:03 +08:00 Compare
张泊明518370910136 force-pushed file_check from e620d22e57 to fc01426a9b 2024-06-20 17:07:43 +08:00 Compare
张泊明518370910136 requested changes 2024-06-21 04:54:33 +08:00
@ -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

check releases need to use Gitea API, just check tags is enough

Yep, only check tags

Yep, only check tags

Where is it tested?

Where is it tested?
bomingzh marked this conversation as resolved
@ -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?

what should be fixed?
Author
Member

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 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.

I think checking an empty value for skipping it is enough. No dir specified = no verify.
zzjc123 marked this conversation as resolved
@ -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)

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

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 and n to this function, we still need to decide the count of hw&proj when generating conf.toml. I do not see much difference.

So since we need to pass `category` and `n` to this function, we still need to decide the count of hw&proj when generating `conf.toml`. I do not see much difference.
zzjc123 marked this conversation as resolved
Author
Member

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.

[[stages]]
name = "healthcheck"
[stages.executor]
name = "sandbox"
[stages.executor.with.default]
args = ["healthcheck", "-root=.", "-whitelist=.gitignore", "-whitelist=currenthw", "-whitelist=.gitattributes", "-whitelist=.drone.yml", "-whitelist=Makefile", "-whitelist=CMakeLists.txt", "-whitelist=.*.js", "-whitelist=.*.json", "-whitelist=conf.toml", "-whitelist=log.txt", "-whitelist=localList", "-whitelist=.*.cpp", "-whitelist=.*.h", "-whitelist=.*.elm", "-whitelist=.*.md", "-whitelist=.*.sh", "-whitelist=.*.query", "-whitelist=.clang-format", "-whitelist=.editorconfig", "-whitelist=executor", "-whitelist=stderr", "-whitelist=stdout", "-localList=localList"]
env = ["PATH=/usr/bin:/bin:/usr/local/bin"]
cpuLimit = 10_000_000_000
memoryLimit = 104_857_600
procLimit = 50
copyInCwd = true
copyOut = ["stdout", "stderr"]
[stages.executor.with.default.copyIn.healthcheck]
src = "/usr/local/bin/healthcheck"
copyOut = ["stdout", "stderr"]
[stages.executor.with.default.stdin]
content = ""
[stages.executor.with.default.stdout]
name = "stdout"
max = 4_096
[stages.executor.with.default.stderr]
name = "stderr"
max = 4_096
[stages.parser]
name = "healthcheck"
[stages.parser.with]
score = 10
comment = " + comment from toml conf"
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. ```toml [[stages]] name = "healthcheck" [stages.executor] name = "sandbox" [stages.executor.with.default] args = ["healthcheck", "-root=.", "-whitelist=.gitignore", "-whitelist=currenthw", "-whitelist=.gitattributes", "-whitelist=.drone.yml", "-whitelist=Makefile", "-whitelist=CMakeLists.txt", "-whitelist=.*.js", "-whitelist=.*.json", "-whitelist=conf.toml", "-whitelist=log.txt", "-whitelist=localList", "-whitelist=.*.cpp", "-whitelist=.*.h", "-whitelist=.*.elm", "-whitelist=.*.md", "-whitelist=.*.sh", "-whitelist=.*.query", "-whitelist=.clang-format", "-whitelist=.editorconfig", "-whitelist=executor", "-whitelist=stderr", "-whitelist=stdout", "-localList=localList"] env = ["PATH=/usr/bin:/bin:/usr/local/bin"] cpuLimit = 10_000_000_000 memoryLimit = 104_857_600 procLimit = 50 copyInCwd = true copyOut = ["stdout", "stderr"] [stages.executor.with.default.copyIn.healthcheck] src = "/usr/local/bin/healthcheck" copyOut = ["stdout", "stderr"] [stages.executor.with.default.stdin] content = "" [stages.executor.with.default.stdout] name = "stdout" max = 4_096 [stages.executor.with.default.stderr] name = "stderr" max = 4_096 [stages.parser] name = "healthcheck" [stages.parser.with] score = 10 comment = " + comment from toml conf" ```
周赵嘉程521432910016 requested review from 张泊明518370910136 2024-08-30 15:32:35 +08:00

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.

[[stages]]
name = "healthcheck"
[stages.executor]
name = "sandbox"
[stages.executor.with.default]
args = ["healthcheck", "-root=.", "-whitelist=.gitignore", "-whitelist=currenthw", "-whitelist=.gitattributes", "-whitelist=.drone.yml", "-whitelist=Makefile", "-whitelist=CMakeLists.txt", "-whitelist=.*.js", "-whitelist=.*.json", "-whitelist=conf.toml", "-whitelist=log.txt", "-whitelist=localList", "-whitelist=.*.cpp", "-whitelist=.*.h", "-whitelist=.*.elm", "-whitelist=.*.md", "-whitelist=.*.sh", "-whitelist=.*.query", "-whitelist=.clang-format", "-whitelist=.editorconfig", "-whitelist=executor", "-whitelist=stderr", "-whitelist=stdout", "-localList=localList"]
env = ["PATH=/usr/bin:/bin:/usr/local/bin"]
cpuLimit = 10_000_000_000
memoryLimit = 104_857_600
procLimit = 50
copyInCwd = true
copyOut = ["stdout", "stderr"]
[stages.executor.with.default.copyIn.healthcheck]
src = "/usr/local/bin/healthcheck"
copyOut = ["stdout", "stderr"]
[stages.executor.with.default.stdin]
content = ""
[stages.executor.with.default.stdout]
name = "stdout"
max = 4_096
[stages.executor.with.default.stderr]
name = "stderr"
max = 4_096
[stages.parser]
name = "healthcheck"
[stages.parser.with]
score = 10
comment = " + comment from toml conf"

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.

> 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. > > ```toml > [[stages]] > name = "healthcheck" > [stages.executor] > name = "sandbox" > [stages.executor.with.default] > args = ["healthcheck", "-root=.", "-whitelist=.gitignore", "-whitelist=currenthw", "-whitelist=.gitattributes", "-whitelist=.drone.yml", "-whitelist=Makefile", "-whitelist=CMakeLists.txt", "-whitelist=.*.js", "-whitelist=.*.json", "-whitelist=conf.toml", "-whitelist=log.txt", "-whitelist=localList", "-whitelist=.*.cpp", "-whitelist=.*.h", "-whitelist=.*.elm", "-whitelist=.*.md", "-whitelist=.*.sh", "-whitelist=.*.query", "-whitelist=.clang-format", "-whitelist=.editorconfig", "-whitelist=executor", "-whitelist=stderr", "-whitelist=stdout", "-localList=localList"] > env = ["PATH=/usr/bin:/bin:/usr/local/bin"] > cpuLimit = 10_000_000_000 > memoryLimit = 104_857_600 > procLimit = 50 > copyInCwd = true > copyOut = ["stdout", "stderr"] > [stages.executor.with.default.copyIn.healthcheck] > src = "/usr/local/bin/healthcheck" > copyOut = ["stdout", "stderr"] > [stages.executor.with.default.stdin] > content = "" > [stages.executor.with.default.stdout] > name = "stdout" > max = 4_096 > [stages.executor.with.default.stderr] > name = "stderr" > max = 4_096 > [stages.parser] > name = "healthcheck" > [stages.parser.with] > score = 10 > comment = " + comment from toml conf" > ``` 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.
张泊明518370910136 requested changes 2024-09-10 16:15:06 +08:00
README.md Outdated
@ -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.

It is outdated now.
zzjc123 marked this conversation as resolved
@ -0,0 +37,4 @@
}
if actualChecksum == expectedChecksum {
return true, "" // fmt.Sprintf("Checksum for %s passed!", filePath)

remove the comment

remove the comment
bomingzh marked this conversation as resolved
@ -0,0 +44,4 @@
}
func VerifyFiles(rootDir string, checkFileNameList string, checkFileSumList string) error {
// Parse command-line arguments

ditto

ditto
bomingzh marked this conversation as resolved
@ -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

remove it
bomingzh marked this conversation as resolved
@ -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

remove it

I mean remove the os.Exit(1), and also the one on line 55.

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

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?

Why so many returns?
张泊明518370910136 reviewed 2024-09-11 05:26:50 +08:00
@ -0,0 +66,4 @@
errorMessages = append(errorMessages, message)
}
}
if allPassed {

why not just return error on not all passed?

why not just return error on not all passed?
张泊明518370910136 force-pushed file_check from 79e42c366e to 102b0ef2a5 2024-09-11 19:46:13 +08:00 Compare
张泊明518370910136 added 1 commit 2024-09-11 20:08:20 +08:00
张泊明518370910136 changed title from WIP: feat: repo health check (#16) to feat: repo health check (#16) 2024-09-11 20:08:43 +08:00
张泊明518370910136 merged commit 5a860f1203 into master 2024-09-11 20:09:27 +08:00
张泊明518370910136 deleted branch file_check 2024-09-11 20:09:28 +08:00
王韵晨520370910012 self-assigned this 2024-09-11 20:45:59 +08:00

merged??? I see many unresolved comments...

merged??? I see many unresolved comments...
Sign in to join this conversation.
No description provided.