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

Add function VM-Get-Category to retrieve the category from the nuspec #1272

Merged
merged 2 commits into from
Feb 14, 2025

Conversation

sara-rn
Copy link
Contributor

@sara-rn sara-rn commented Feb 10, 2025

Retrieves the category from the nuspec file through the helper function VM-Get-Category
Make lint.py compatible with the new nuspec format: add tags to the list of required tags and validate categories from the nuspec file
Closes #1107

@sara-rn sara-rn requested a review from Ana06 February 10, 2025 12:59
@sara-rn sara-rn changed the title Add new function VM-Get-Category to retrieve the category from the nu… Add function VM-Get-Category to retrieve the category from the nuspec Feb 10, 2025
@sara-rn sara-rn self-assigned this Feb 10, 2025
@sara-rn sara-rn force-pushed the add-vm-get-category branch 3 times, most recently from 334ab62 to 42b0194 Compare February 10, 2025 13:26
Copy link
Member

@Ana06 Ana06 left a comment

Choose a reason for hiding this comment

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

Nice first step to move the categories to the nuspec! 😄

@sara-rn sara-rn force-pushed the add-vm-get-category branch from 07f3d18 to e9754f3 Compare February 11, 2025 09:47
# Returns the category if the tags exist in the nuspec
function VM-Get-Category {
$installPath = $MyInvocation.MyCommand.Definition
$toolDir = "$(Split-Path -parent (Split-Path -parent $installPath))"
Copy link
Member

Choose a reason for hiding this comment

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

According to our documentation $toolDir should only be used for directory where the tool is actually installed. We need a different name here. I think you can also remove the "

Suggested change
$toolDir = "$(Split-Path -parent (Split-Path -parent $installPath))"
$chocolateyToolDir = $(Split-Path -parent (Split-Path -parent $installPath))

function VM-Get-Category {
$installPath = $MyInvocation.MyCommand.Definition
$toolDir = "$(Split-Path -parent (Split-Path -parent $installPath))"
$packageName = Split-Path -Path (Split-Path -parent $installPath) | Split-Path -Leaf
Copy link
Member

Choose a reason for hiding this comment

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

you can get the package name from the environment variable ChocolateyPackageName

$toolDir = "$(Split-Path -parent (Split-Path -parent $installPath))"
$packageName = Split-Path -Path (Split-Path -parent $installPath) | Split-Path -Leaf
$nuspec = $packageName + ".nuspec"
if (Test-Path ($toolDir)){
Copy link
Member

Choose a reason for hiding this comment

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

the directory must exist, otherwise we return an invalid category here

Copy link
Contributor Author

@sara-rn sara-rn Feb 11, 2025

Choose a reason for hiding this comment

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

@Ana06 if $toolDir doesn't exist, the return value should be None?

Copy link
Member

@Ana06 Ana06 Feb 12, 2025

Choose a reason for hiding this comment

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

how is it possible that it does not exists? I think it MUST exists and you should check it with VM-Assert-Path or -Resolve which would raise an exception if it does not exist.

Copy link
Contributor Author

@sara-rn sara-rn Feb 12, 2025

Choose a reason for hiding this comment

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

ah ok, so I remove the condition Test-Path, I misunderstood your comment, thanks!

@sara-rn sara-rn force-pushed the add-vm-get-category branch 2 times, most recently from 42b0194 to 7576727 Compare February 11, 2025 11:00
@@ -94,6 +94,7 @@ class IncludesRequiredFieldsOnly(Lint):
"description",
"authors",
"dependencies",
"tags",
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this change and add it after all packages have the category in the nuspec.

scripts/test/lint.py Show resolved Hide resolved
@@ -3,7 +3,7 @@ Import-Module vm.common -Force -DisableNameChecking

try {
$toolName = '010Editor'
$category = 'Hex Editors'
$category = VM-Get-Category($MyInvocation.MyCommand.Definition)
Copy link
Member

Choose a reason for hiding this comment

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

Documenting discussion: It is a good idea to edit two packages (common and an extra one) in this PR to test VM-Get-Category. But it is a good idea to split the update of other packages in separate PR (for example 30 packages per PR) so that the CI takes less to run and we do not encounter any issues when pushing the packages to MyGet.

Copy link
Member

Choose a reason for hiding this comment

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

Note the issue with 010Editor (just in case you notice slow local testing or CI): #1275

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ana06 2 test packages have been removed from current PR

@sara-rn sara-rn force-pushed the add-vm-get-category branch 6 times, most recently from ec6ff24 to e48bcf5 Compare February 13, 2025 10:16
@sara-rn sara-rn requested a review from Ana06 February 13, 2025 10:23
Copy link
Member

@Ana06 Ana06 left a comment

Choose a reason for hiding this comment

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

[nipick] Remember to keep the commit subject short (50 chars is a common convention):
image

Also update the commit description, it mentions 7z which is not being updated.

scripts/test/lint.py Show resolved Hide resolved
packages/common.vm/tools/vm.common/vm.common.psm1 Outdated Show resolved Hide resolved
@sara-rn sara-rn force-pushed the add-vm-get-category branch 3 times, most recently from ed87abc to 90c6e5a Compare February 14, 2025 09:45
@sara-rn sara-rn requested a review from Ana06 February 14, 2025 09:50
Retrieves the category from the nuspec file through the helper function `VM-Get-Category`
Make lint.py compatible with the new nuspec format:
add tags to the list of required tags and validate categories from the nuspec file
Comment out the line that validates the categories from the chocolatey install package.
This needs to be termporarily disabled until the new linter is ready and all the
packages have the new structure.
This is needed because the choco install package won't longer have a category,
it will read it from the nuspec instead.
@sara-rn sara-rn force-pushed the add-vm-get-category branch from 90c6e5a to 944cddf Compare February 14, 2025 15:47
@Ana06 Ana06 merged commit a204833 into mandiant:main Feb 14, 2025
4 checks passed
@sara-rn sara-rn deleted the add-vm-get-category branch February 17, 2025 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move the categories to the nuspec & Add wiki page with tools by categories
2 participants