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

feat: Release improved project naming for csproj behind a flag #456

Merged
merged 1 commit into from
Apr 28, 2019

Conversation

lili2311
Copy link
Contributor

@lili2311 lili2311 commented Apr 25, 2019

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

What does this PR do?

Bump the nuget plugin to release improved naming behind the flag --assets-project-name
snyk monitor --file=pathtosln --org=snykorg --assets-project-name
snyk/snyk-nuget-plugin#50

@adrukh
Copy link
Contributor

adrukh commented Apr 25, 2019

@lili2311 do we want to refer to this shiny new flag in the help text?

@lili2311 lili2311 force-pushed the feat/optional-proj-names-nuget branch 2 times, most recently from f37c0af to 27ceaa7 Compare April 25, 2019 16:06
@lili2311 lili2311 closed this Apr 26, 2019
@lili2311 lili2311 reopened this Apr 26, 2019
help/help.txt Outdated
@@ -56,6 +56,9 @@ Options:
test a specific sub-project.
--all-sub-projects For "multi project" configurations, test all
sub-projects.
--assets-project-name
For Nuget projects, forces us to try using the project name
from the project.assets.json file when possible.
Copy link
Contributor

Choose a reason for hiding this comment

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

For Nuget project, forces us to try using the project name from the project.assets.json file when possible.

While very accurate and true, I'm still left a bit puzzled - what happens when I don't use this option? Also, the use of forces us is a bit off to me :)

Suggestion:

When monitoring a .NET project, use the project.assets.json name, if found.

Choose a reason for hiding this comment

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

We might want to specify .NET project using NuGet PackageReference project style.
If you scan a .NET framework that use packages.config insteand of PackageReference, the argument will have no effects.

Also, we could specify that we check for the projectname in project.assets.json or something like that. "Use the project.assets.json project name" might be more accurate

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we actually move this documentation to the plugin?

We have many options that are plugin specific, and that is where they should live until we decide otherwise.

@lili2311 lili2311 force-pushed the feat/optional-proj-names-nuget branch from 27ceaa7 to 49ad843 Compare April 28, 2019 09:26
-h, --help ......... This help information.
-v, --version ...... The CLI version.

Gradle options:
--gradle-sub-project=<string>
For Gradle "multi project" configurations,
test a specific sub-project.
--all-sub-projects For "multi project" configurations, test all
Copy link
Contributor

Choose a reason for hiding this comment

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

this one is CLI-wide. but only gradle uses it at the moment. It's fine here now, but we should remember to move it when we support other PM's like yarn workspaces etc.

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.

I like this separation. @adrukh what do you think?

Copy link
Contributor

@adrukh adrukh left a comment

Choose a reason for hiding this comment

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

Big 👍

@lili2311 lili2311 merged commit a21cd0c into master Apr 28, 2019
@lili2311 lili2311 deleted the feat/optional-proj-names-nuget branch April 28, 2019 14:45
@snyksec
Copy link

snyksec commented Apr 28, 2019

🎉 This PR is included in version 1.154.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

tommyknows added a commit that referenced this pull request Sep 14, 2022
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