Skip to content
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.

Sign up for GitHub

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

Fix dependency names on state commit #3447

Merged
merged 7 commits into from
Aug 16, 2024
Merged

Fix dependency names on state commit #3447

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
669fbcd
Commit WIP
MDrakos Aug 12, 2024
e9f0904
Merge branch 'version/0-46-0-RC1' into DX-2966
MDrakos Aug 13, 2024
5164883
Merge branch 'version/0-46-0-RC1' into DX-2966
MDrakos Aug 14, 2024
a21d536
Reorganize
MDrakos Aug 14, 2024
89f2114
Move requirement logic inside report function
MDrakos Aug 14, 2024
787aafa
Use buildplan for requirements calculation
MDrakos Aug 15, 2024
1e2073d
Merge branch 'version/0-46-0-RC1' into DX-2966
MDrakos Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 56 additions & 1 deletion internal/runners/commit/commit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/ActiveState/cli/internal/runbits/cves"
"github.com/ActiveState/cli/internal/runbits/dependencies"
"github.com/ActiveState/cli/internal/runbits/rationalize"
"github.com/ActiveState/cli/pkg/buildscript"
"github.com/ActiveState/cli/pkg/localcommit"
bpResp "github.com/ActiveState/cli/pkg/platform/api/buildplanner/response"
"github.com/ActiveState/cli/pkg/platform/model/buildplanner"
Expand Down Expand Up @@ -159,7 +160,19 @@ func (c *Commit) Run() (rerr error) {
dependencies.OutputChangeSummary(out, rtCommit.BuildPlan(), oldCommit.BuildPlan())

// Report CVEs.
if err := cves.NewCveReport(c.prime).Report(rtCommit.BuildPlan(), oldCommit.BuildPlan()); err != nil {
newReqs, err := newRequirements(oldCommit.BuildScript(), rtCommit.BuildScript())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't feel right, because we're going to need to duplicate this logic in other places that will eventually get CVE reporting. It looks like state import is not passing any names to cves.NewCveReport() either, so presumably it suffers from the same bug.

Can we put all of this logic in the Report() method? Kind of like how the change summary reporter diffs two buildplans? (

cli/internal/runbits/dependencies/changesummary.go

Line 30 in 0bb4d84

changeset := newBuildPlan.DiffArtifacts(oldBuildPlan, false)
). If we need absolutely need buildscripts, then maybe we should be passing commits to the method instead of buildplans.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point about the repeated logic. The changeset gets us back to the original problem where it's including too many requirements as we just want the top level ones and not the dependencies. I've moved this logic inside the Report method and we're now passing it commits.

if err != nil {
return errs.Wrap(err, "Could not get new requirements")
}
var names []string
for _, req := range newReqs {
req, ok := req.(buildscript.DependencyRequirement)
if !ok {
continue
}
names = append(names, req.Name)
}
if err := cves.NewCveReport(c.prime).Report(rtCommit.BuildPlan(), oldCommit.BuildPlan(), names...); err != nil {
return errs.Wrap(err, "Could not report CVEs")
}

Expand All @@ -181,3 +194,45 @@ func (c *Commit) Run() (rerr error) {

return nil
}

func newRequirements(oldBuildScript *buildscript.BuildScript, newBuildScript *buildscript.BuildScript) ([]buildscript.Requirement, error) {
var requirements []buildscript.Requirement

old, err := oldBuildScript.Requirements()
if err != nil {
return nil, errs.Wrap(err, "Could not get old requirements")
}

oldReqs := make(map[string]bool)
for _, req := range old {
req, ok := req.(buildscript.DependencyRequirement)
if !ok {
continue
}
oldReqs[qualifiedName(req)] = true
}

newReqs, err := newBuildScript.Requirements()
if err != nil {
return nil, errs.Wrap(err, "Could not get new requirements")
}

for _, req := range newReqs {
req, ok := req.(buildscript.DependencyRequirement)
if !ok {
continue
}
if !oldReqs[qualifiedName(req)] {
requirements = append(requirements, req)
}
}

return requirements, nil
}

func qualifiedName(req buildscript.DependencyRequirement) string {
if req.Namespace == "" {
return req.Name
}
return req.Namespace + "/" + req.Name
}
Loading