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.

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

Use buildplanner ImpactReport endpoint to show change summary. #3419

Merged
merged 8 commits into from
Aug 1, 2024

Conversation

mitchell-as
Copy link
Contributor

@mitchell-as mitchell-as commented Jul 29, 2024

StoryDX-2955 Our change summary and cve summary use the buildplanner ImpactReport

The ImpactReport sometimes has issues resolving one or both buildplans, so fall back on the old comparison if necessary.
Commits are not guaranteed to belong to projects yet (they could be local).
Comment on lines 12 to 13
BeforeExpr []byte
AfterExpr []byte
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot use commit IDs because at this time they must be publicly attached to the project. Local commit IDs will not work. I've filed PB-5177 for this.

@mitchell-as mitchell-as requested a review from Naatan July 30, 2024 18:17
@mitchell-as mitchell-as marked this pull request as ready for review July 30, 2024 18:17
Copy link
Member

@Naatan Naatan left a comment

Choose a reason for hiding this comment

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

In addition to flagged code, I think this should also be updating this code here:

changeset := newBuildPlan.DiffArtifacts(oldBuildPlan, false)

I'd also expect the DiffArtifacts() function to be dropped as part of this PR.

Comment on lines 33 to 53
// Fetch the impact report.
beforeExpr, err := json.Marshal(oldCommit.BuildScript())
if err != nil {
return errs.Wrap(err, "Unable to marshal old buildexpression")
}

afterExpr, err := json.Marshal(newCommit.BuildScript())
if err != nil {
return errs.Wrap(err, "Unable to marshal buildexpression")
}
bpm := buildplanner.NewBuildPlannerModel(prime.Auth())
params := &buildplanner.ImpactReportParams{
Owner: prime.Project().Owner(),
Project: prime.Project().Name(),
BeforeExpr: beforeExpr,
AfterExpr: afterExpr,
}
report, err := bpm.ImpactReport(params)
if err != nil {
return errs.Wrap(err, "Failed to fetch impact report")
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a model function for this that abstracts all of this away? We'll be doing the same thing in cves.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've also changed the change and cve reports to accept an impact report so it's only computed once.

continue
}

if i.Before == nil {
Copy link
Member

Choose a reason for hiding this comment

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

May as well pair this up with the identical conditional on line 64.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would, but they're not identical 😱

@mitchell-as mitchell-as force-pushed the mitchell/dx-2955 branch 3 times, most recently from 9ae9268 to 7319be3 Compare July 30, 2024 21:12
…ully utilized.
@mitchell-as
Copy link
Contributor Author

Test failures are a mix of known issues and timeouts. They are not caused by this PR.

@mitchell-as mitchell-as requested a review from Naatan July 30, 2024 22:34
type ImpactReportParams struct {
Owner string
Project string
Before buildScripter
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using *buildplanner.Commit would result in an import cycle.

Copy link
Member

Choose a reason for hiding this comment

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

Assuming there isn't an import cycle with buildscripts you could pass in the buildscript instead? It looks like that's all you're accessing on the commit.

I'd like to find a way to address this without interfaces if we can find a feasible solution.

Copy link
Member

@Naatan Naatan left a comment

Choose a reason for hiding this comment

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

Looking good! I think we're almost there.

Comment on lines 61 to 63
if !c.prime.Auth().Authenticated() || len(ingredients) == 0 {
logging.Debug("Skipping CVE reporting")
return nil
Copy link
Member

Choose a reason for hiding this comment

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

Can we put the auth check at the top and the len() check here? For the len check we don't need a debug entry.

This is pretty nitpicky I realize but it feels wrong to me to have both fire the same debug log, and especially for there not being any ingredients it doesn't seem worth logging at all.


buildPlan := newCommit.BuildPlan()

func OutputChangeSummary(out output.Outputer, report *response.ImpactReportResult, rtCommit *buildplanner.Commit) {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we only use rtCommit to retrieve the buildplan, so we should just pass the buildplan here.

type ImpactReportParams struct {
Owner string
Project string
Before buildScripter
Copy link
Member

Choose a reason for hiding this comment

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

Assuming there isn't an import cycle with buildscripts you could pass in the buildscript instead? It looks like that's all you're accessing on the commit.

I'd like to find a way to address this without interfaces if we can find a feasible solution.

@mitchell-as mitchell-as requested a review from Naatan July 31, 2024 21:08
Copy link
Member

@Naatan Naatan left a comment

Choose a reason for hiding this comment

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

Nice work!

@mitchell-as mitchell-as closed this Aug 1, 2024
@mitchell-as mitchell-as reopened this Aug 1, 2024
@mitchell-as mitchell-as merged commit 1d66f33 into version/0-46-0-RC1 Aug 1, 2024
10 of 13 checks passed
@mitchell-as mitchell-as deleted the mitchell/dx-2955 branch August 1, 2024 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants