feat: [SUP-2164] Change dependency graph root name logic #266
Conversation
|
|
lib/index.ts
Outdated
| projectName = isValidRootDir | ||
| ? `${path.basename(root)}/${projectId}` | ||
| : `${defaultProject}/${projectId}`; | ||
| projectName = `${defaultProject}/${projectId}`; |
There was a problem hiding this comment.
There was a problem hiding this comment.
Note that this is not the actual projectName that is being changed, despite the variable name. It's the root package name only.
Actual projectName stays as it is.
There was a problem hiding this comment.
If we accept it we should probably unify it in a function.
There was a problem hiding this comment.
Should the variable be named to something other than projectName to avoid future confusion?
dotkas
force-pushed
the
dotkas/SUP-2164/test-monitor-projectname-discrepancy
branch
6 times, most recently
from
6245cd9 to
6b879dd
Compare
dotkas
force-pushed
the
dotkas/SUP-2164/test-monitor-projectname-discrepancy
branch
2 times, most recently
from
2ef19e0 to
8a7806f
Compare
BREAKING CHANGE: This major version bump is mostly cautionary, as existing consumers of this plugin ought not to encounter errors. However, it is a change in how the makeup of the dependency graph works, and therefore could be argued to require this version bump. Thus we err on the side of caution.
dotkas
force-pushed
the
dotkas/SUP-2164/test-monitor-projectname-discrepancy
branch
from
8a7806f to
7aabd6e
Compare
| targetFile | ||
| ${'build.gradle'} | ||
| ${'build.gradle'} | ||
| ${'build.gradle'} | ||
| ${path.join(fakeRootDir, 'build.gradle')} |
There was a problem hiding this comment.
rootDir was not needed as we're no longer using it for rootPkg name generation.
dotkas
force-pushed
the
dotkas/SUP-2164/test-monitor-projectname-discrepancy
branch
from
fed5724 to
80c24c7
Compare
There was a problem hiding this comment.
Seems the pipeline has been failing for a long time, so most likely unrelated to this change.
package.json
Outdated
There was a problem hiding this comment.
Beyond this points it's all updated fixtures.
dotkas
requested review from
danielroymoore
and removed request for
klesniewski and
danlucian
dotkas
deleted the
dotkas/SUP-2164/test-monitor-projectname-discrepancy
branch
|
🎉 This PR is included in version 4.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What this does
There is a discrepancy between what we call our projects and what we call our root nodes in the dependency graphs. This sounds like it should not matter, but it does.
We have a regression introduced after a bugfix was made last month, where
snyk testnow does not correctly pick up ignores in the ui after asnyk monitoron certain types of Gradle projects.Doing this change will fix that.
Problem
(Links to internal projects)
monitor.test.Since the bugfix last month, root package names no longer match the project names in Gradle projects. Doing a
--list-depsor looking at the UI will show one name in the project name and another in the root package list.This is the root cause of the bug why
snyk testdoes not pick up changes, as the project identity that is being tested against is generated from the root package name, not the "defaultProjectName" likesnyk monitor.Breaking change
I'm bumping the major version for this one, unless anybody objects. Not technically a breaking change, as this should not cause orphaned projects. It should merely create a new element in the history i the same project - but out of caution i find it makes sense, just to show that we're changing expected behavior. In case any consumer is relying on this value.
Project identity?
The
projectNameshould not change because of these changes. This change actually more aligns the root package name to what is already done for project names: https://github.com/snyk/snyk-gradle-plugin/blob/a444194f1199afece07232f4568773f36f9bfb9f/lib/index.ts#L306-L316Snapshot churn?
This should change dependency graphs for CLI monitor projects, and only a part of Gradle customers, so after discussing with some stakeholders in Team Fix, have deemed it should not be dangerous to snapshot churn.