✨ feat: Refactor scorecard serve cmd #4665
Conversation
Fix3dP0int
changed the title
|
Don't have time to look in-depth, but wanted to make one comment:
What features specifically? Go 1.22 made good strides at least for the routing |
|
Sorry. I'm unfamiliar with new features of Go. It seems that |
Signed-off-by: fixedpoint <961750412@qq.com>
Fix3dP0int
requested review from
spencerschrock and
raghavkaul
and removed request for
a team
There was a problem hiding this comment.
At a high level this looks like what we want, an http wrapper around the CLI. You said MCP will be built on top of this API, so is it matching what you need for that?
There are a few things that need changed, which I've left individual comments on. The linter also has some thoughts with this file.
| PolicyFile string `json:"policy_file,omitempty"` | ||
| } |
There was a problem hiding this comment.
In the CLI, policy file is a local file. How do we expect to pass a policy file to a server, with a URI?
| if r.Method == http.MethodPost { | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| http.Error(w, "invalid request body", http.StatusBadRequest) | ||
| return | ||
| } |
There was a problem hiding this comment.
what is the goal of having POST? Passing parameters as JSON instead of as query parameters?
| if s.opts.LogLevel == "" { | ||
| s.opts.LogLevel = "info" |
There was a problem hiding this comment.
log.InfoLevel is a constant with this value.
| // Set options | ||
| s.opts.Repo = req.Repo | ||
| s.opts.Local = req.Local | ||
| s.opts.NPM = req.NPM | ||
| s.opts.PyPI = req.PyPI |
There was a problem hiding this comment.
Right now we have one options.Options, which is passed in serveCmd, and then this one copy is modified for each request. This seems like a race condition.
we should create a new struct for each request. either through options.New, or manually if we want to avoid the env var parsing.
| } else if s.opts.FileMode != options.FileModeArchive && s.opts.FileMode != options.FileModeGit { | ||
| http.Error(w, fmt.Sprintf("unsupported file mode: %s", s.opts.FileMode), http.StatusBadRequest) | ||
| return | ||
| } |
There was a problem hiding this comment.
this validation is already covered by the s.opts.Validate call below
| if s.opts.Local != "" { | ||
| repo, err = localdir.MakeLocalDirRepo(s.opts.Local) | ||
| if err != nil { | ||
| http.Error(w, fmt.Sprintf("making local dir: %v", err), http.StatusInternalServerError) | ||
| return | ||
| } |
There was a problem hiding this comment.
do we expect people to pass the serve command repos which are local to the server?
| enabledChecks, err := policy.GetEnabled(pol, s.opts.Checks(), requiredRequestTypes) | ||
| stdlog.Printf("DEBUG: enabledChecks = %#v", enabledChecks) |
There was a problem hiding this comment.
should this be removed? was this for your testing?
| Details: s.opts.ShowDetails, | ||
| Annotations: false, |
There was a problem hiding this comment.
we would probably want ShowAnnotations as a query parameter.
Fix3dP0int
temporarily deployed
to
integration-test
GitHub Actions
Inactive
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| start := time.Now() | ||
| next.ServeHTTP(w, r) | ||
| stdlog.Printf("%s %s %s", r.Method, r.URL, time.Since(start)) |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4665 +/- ##
==========================================
+ Coverage 66.80% 67.71% +0.91%
==========================================
Files 230 249 +19
Lines 16602 19044 +2442
==========================================
+ Hits 11091 12896 +1805
- Misses 4808 5289 +481
- Partials 703 859 +156 🚀 New features to boost your workflow:
|
What kind of change does this PR introduce?
feature
What is the current behavior?
The current serve uses Go’s built-in http package, which lacks modern features. And It fails to correctly aggregate the total score, and parameters and details cannot be retrieved properly.
What is the new behavior (if this is a feature change)?**
This PR refactors the serve component by migrating the original CLI-based parameter input to a RESTful API interface. Additionally, I replaced the native net/http logic with the chi router, which is lightweight yet expressive and well-suited for modular HTTP services in Go.
Which issue(s) this PR fixes
Related to #4627