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

Front-End for GitHub PR Generation #1134

Open
wants to merge 5 commits into
base: file-github-pr-resolver
Choose a base branch
from

Conversation

freeqaz
Copy link
Member

@freeqaz freeqaz commented Feb 17, 2023

#1030
This is tested and it works, but we still need to write the final part of the UI. (Showing the link to the PR that was created.)

For now, this does work and it just needs some additional polish to be shipped.

We will need to increase the permissions that we ask for with the GitHub app, unfortunately, because we don't currently have write
permissions to repositories associated with the GitHub App.

Maybe there is a way that we can progressively do that to let people opt-in? That's a discussion for later.

This is tested and it works, but we still need to write the final part of the UI. (Showing the link to the PR that was created.)

For now, this does work and it just needs some additional polish to be shipped.

We will need to increase the permissions that we ask for with the GitHub app, unfortunately, because we don't currently have write
permissions to repositories associated with the GitHub App.

Maybe there is a way that we can progressively do that to let people opt-in? That's a discussion for later.
Copy link
Contributor

@factoidforrest factoidforrest left a comment

Choose a reason for hiding this comment

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

I requested some changes here

});

// TODO: Make this actually put some HTML on the page.
console.log('pr response:', response);
Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably finish this feature before merging to master.

It would be nice to keep master in a deployable state at all times (not that we succeed, lol)

@@ -113,11 +118,25 @@ export const VulnerablePackageMain: React.FunctionComponent<VulnerablePackageMai
);
};

async function onClickUpdate(pkg: VulnerablePackage) {
const response = await createGitHubPullRequestForVuln({
project_id: build.project_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that this should almost certainly take an array of changes. Clicking this button 5 times and filing 5 PRs for a single build is just not usable, there's no reason to write it in an incomplete way as an MVP since that's missing the "viable" part, IMO. It's nice to have the option to do it both one at a time, or for everything. It's not like that will be significantly more difficult. That's a pretty easy change, from what I've seen so far. There are all the components for that in the frontend kicking around still, I think, since that's how I had it written before.

Also, the "recommended version" is, I don't think, necessarily what we want to update to. That is just the highest fix version as you have it, which is not necessarily the fix version that is the target for the update.

Take a look at the backend code that calculates what the potential patch version is and you'll see it returns the target updatable version to the frontend.

const triviallyUpdatableTo = precomputeVulnTriviallyUpdatableTo(node.range, vulnMeta);
This is what you should be targetting as an update version. So you already have what you need, you can just use that value.

type BuildData_AffectedByVulnerability {
    vulnerability: BuildData_Vulnerability!
    beneath_minimum_severity: Boolean!
    ranges: [BuildData_Range!]!
    ignored: Boolean!
    ignored_vulnerability: BuildData_IgnoredVulnerability
    trivially_updatable_to: String   <---- RIGHT HERE
    fix_versions: [String!]!
    path: String!
    adjustment: BuildData_Adjustment #optional
}

I think, very soon, we will want to change your backend implementation to use its own copy of the tree to make decisions, rather than just passing the data through the frontend. To build the "deep update" feature that I have mentioned several times, we need to be tree-aware anyway. But, for now, You should be able to get away with using the value that I'm passing here trivially_updatable_to

Copy link
Contributor

Choose a reason for hiding this comment

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

nevermind on that last tree part, but still yes on everything else. We can add the "parent update" logic to the backend as well and just pass that alongside the "trivially_updatable_to" that we are already passing here.

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