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

deps(snyk): update snyk snapshot #13823

Merged
merged 3 commits into from
Apr 19, 2022
Merged

deps(snyk): update snyk snapshot #13823

merged 3 commits into from
Apr 19, 2022

Conversation

snyk-bot
Copy link
Contributor

@snyk-bot snyk-bot commented Apr 6, 2022

Why this PR?

a weekly update of the vulnerabilities snapshot for lighthouse

@connorjclark
Copy link
Collaborator

@aviadatsnyk any idea why snyk-bot keeps opening PRs with merge conflicts?

@leeyashalti
Copy link

@aviadatsnyk any idea why snyk-bot keeps opening PRs with merge conflicts?

Hi @connorjclark, I'm Leeya from Snyk. We were not aware of these conflicts, sorry for that.
We'll fix them on our end in the upcoming week.

@MichaelAquilina
Copy link

Hi @connorjclark. I've looked into the issue and it seems like the data is correct.

We think the issue is that git cannot automatically resolve the merge on its own due the two lines being very similar to one another. Specifically, it cannot understand that what is actually happening is a line delete + line update because it cannot figure out which line was deleted and which one was updated. However this is fairly obvious to us as humans because we can look at the id.

The data being proposed from our branch has been confirmed correct. All you need to do is resolve the conflict manually since git cannot figure it out on its own.

This should be a very rare event and once this has been resolved you should not see any further merge conflicts.

@adamraine
Copy link
Member

This should be a very rare event and once this has been resolved you should not see any further merge conflicts.

I did this recently (#13731) which stopped the conflicts for at least one PR (#13774), but the merge conflicts popped up again here. Perhaps they are more common than you think, or maybe just a coincidence.

@MichaelAquilina
Copy link

@adamraine from what I can tell, the mostly likely case for this to occur is when we "merge" two vulnerabilities into one when we notice one is a duplicating the other. This is what happened in this PR for example.

I don't have any numbers on me for how frequently we perform this, but my initial suspicions were that it would be quite low. Unfortunately I do not think there is any other way to handle these specific cases other then manually resolving the conflict from your end when git can not do it automatically.

@connorjclark
Copy link
Collaborator

connorjclark commented Apr 13, 2022

Thanks for looking into it. I've manually fixed the conflict, hopefully futures PRs don't have the problem.

We think the issue is that git cannot automatically resolve the merge on its own due the two lines being very similar to one another.

My assumption is that the snyk PR process works like: snyk has new data, pulls down our latest master repo, branches from it, runs our update-vuln-snapshot.sh then sends a PR. That should just work, and if master was the latest code from the remote then there can be no conflicts.

The bug is probably somewhere in the "pull down the latest master" step, a git status after that step might show git already in an unclean state. For scripts I've written that operate on a git repo in CI I typically do a git reset --hard and git clean -fd just before grabbing the latest code, like here – that prevents any caches from breaking when the last job left some files in an undesired state different from origin/master.

@MichaelAquilina
Copy link

MichaelAquilina commented Apr 13, 2022

@connorjclark maybe worth explaining what we do in a nutshell. This is roughly the process:

  • pull latest master
  • open a new branch
  • regenerate the snapshot.json file according to the state of our database
  • overwrite the existing snapshot.json file with the new file
  • commit and push the changes
  • open a PR via github's API

Because we regenerate the file each time, I think we can always assume that what we have generated and are proposing to merge is the correct data. Sometimes however, as what happened in this PR, git cant understand those differences because some lines are too similar for it to understand what has been deleted and what has been updated. In that scenario, git will require some manual intervention.

Hopefully that makes sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants