-
-
Notifications
You must be signed in to change notification settings - Fork 164
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
base: file-github-pr-resolver
Are you sure you want to change the base?
Conversation
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.
There was a problem hiding this 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); |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
#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.