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

Ability to use project name from project.assets.json file #50

Merged
merged 5 commits into from
Apr 25, 2019
Merged

Ability to use project name from project.assets.json file #50

merged 5 commits into from
Apr 25, 2019

Conversation

nrodrigues1
Copy link
Contributor

@nrodrigues1 nrodrigues1 commented Apr 13, 2019

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What does this PR do?

I'm not a JavaScript programmer, please be cool :)

When using "PackageReference" as NuGet package format (project.assets.json), we should use the project name in project.assets.json. This value represents the name of the csproj file, that is, the name we see in Visual Studio.

For example, if you have a project named MyProject:
"project": { "version": "1.0.0", "restore": { "projectUniqueName": "path to csproj", "projectName": "MyProject", "projectPath": "path to csproj", "packagesPath": "path to nuget package cache", "outputPath": "obj folder path",

By default, we use the folder name where the csproj resides. The problem is that if you have a solution containing a lot of projects where multiple projects are under the same folder name, we only get results for the last scanned project.

Example:
projectpath1\Project1\Tests\project1.test.csproj
projectpath2\Project2\Tests\project2.test.csproj

This will be 1 project in Snyk -> Tests

The same behavior could probably happen with csproj using packages.config

Since we don't want to break any existing setups (even if they can be actually broken), I decided to add a command line parameter named "--assets-project-name". I'm open using something else.

For example:
snyk monitor --file=pathtosln --org=snykorg --assets-project-name

…e. If you have a big solution where, for example, unit tests for all project are under unit tests folder, snyk will only check the last project it had scanned.

Since, we don't want to broke any setups, use "--assets-project-name" switch like:

snyk monitor --file=./JobHandlers/VC12.0/JobHandlers.sln --org=coveo-connectors --assets-project-name
@CLAassistant
Copy link

CLAassistant commented Apr 13, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@orsagie orsagie left a comment

Choose a reason for hiding this comment

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

Do you think there should be a flag for this behaviour in the future? Would it be more correct to always take the project name from the assets file?

Disclaimer: we wont release this without the flag as it may break existing projects.

README.md Show resolved Hide resolved
lib/nuget-parser/index.js Outdated Show resolved Hide resolved
lib/nuget-parser/package-reference-parser.js Outdated Show resolved Hide resolved
lib/nuget-parser/package-reference-parser.js Outdated Show resolved Hide resolved
@nrodrigues1
Copy link
Contributor Author

Do you think there should be a flag for this behaviour in the future? Would it be more correct to always take the project name from the assets file?

Disclaimer: we wont release this without the flag as it may break existing projects.

Hum, I'm confused. I added the argument "--assets-project-name" to not break the existing projects.
Also, I think that this feature should be the default behavior when using PackageReference package (aka project.assets.json) OR the name of the csproj file.

@nrodrigues1
Copy link
Contributor Author

@orsagie Will you have some time to check the PR this week?

@orsagie
Copy link
Contributor

orsagie commented Apr 25, 2019

It looks good now. Approved!

Hum, I'm confused. I added the argument "--assets-project-name" to not break the existing projects.
Also, I think that this feature should be the default behavior when using PackageReference package (aka project.assets.json) OR the name of the csproj file.

The question about the flag was to gauge if this is a common enough problem that we might not want it behind a flag in the future. Currently the flag is necessary.

@nrodrigues1
Copy link
Contributor Author

@orsagie Cool thank you!

Can you merge it since I don't have write access in this repo?

Thanks!

@lili2311 lili2311 merged commit c97a365 into snyk:master Apr 25, 2019
@lili2311
Copy link
Contributor

Merged :) I will see it make it's way to a new version of the CLI

@snyksec
Copy link

snyksec commented Apr 25, 2019

🎉 This PR is included in version 1.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

5 participants