-
Notifications
You must be signed in to change notification settings - Fork 88
Suricate Package Addition #1375
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
base: main
Are you sure you want to change the base?
Conversation
Ana06
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PrajeetGuha as we need this change for Suricata, I think we should add the package in the same PR (if you wish in a different commit) so that we can test them together.
You need to rebase to solve the conflict with the package version.
Ana06
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PrajeetGuha I see some activity in the PR, but I do not see the new package. @PrajeetGuha have you maybe forgotten to push the changes? 🤔 Let me know if you need help with anything.
|
For the package to work, I had to install The github action workflow will fail due to absence of the powershell module. |
I have just updated the wiki, thanks for bringing it up! |
Ana06
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work @PrajeetGuha! 💐
All code that could fail needs to be inside a try-catch with a VM-Write-Log-Exception $_ to better handle exceptions. It is always good to have new people contributing to the project, as we notice where we are lucking documentation. I have just documented the try-catch in https://github.com/mandiant/VM-Packages/wiki/Coding-Conventions. 😉
Thanks!! Will check and add exceptions wherever necessary after the code is working as expected and is approved. 🫡 |
This should be easy to address. Normally all packages have just a single |
900bbe9 to
13f9f60
Compare
Ana06
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Param( | ||
| [Parameter(Mandatory=$true, Position=0)] | ||
| [ValidateSet("INFO","WARN","ERROR")] | ||
| [ValidateSet("INFO","WARN","ERROR","FATAL")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand this change. 😕 Can you please explain me why this is needed @PrajeetGuha?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (($level -eq "ERROR") -Or ($level -eq "FATAL")) { |
has $level=FATAL but the ValidateSet does not have it. This change is not related to this package implementation though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] It is a good practice to not commit unrelated changes in the same commit. But I see why this is an enhancement now. 😉
|
Google CLA test doesn't like my personal email as co-author. I think using @PrajeetGuha did you add this manually or was it the GH interface? I would like to try to configure the email if it was the GH interface. |
It is the GH interface. I think it will not be problem when I squash and rebase the code after final review. Let me know if any changes are required. |
8da3845 to
985fab2
Compare
Ana06
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PrajeetGuha so sorry for the late response, I had completely missed that you had made the changes 🙈
I have just tested locally and the suricata icon seems to be broken:

| Param( | ||
| [Parameter(Mandatory=$true, Position=0)] | ||
| [ValidateSet("INFO","WARN","ERROR")] | ||
| [ValidateSet("INFO","WARN","ERROR","FATAL")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] It is a good practice to not commit unrelated changes in the same commit. But I see why this is an enhancement now. 😉
It's fine 🫡
Unfortunately with the present implementation, the installer cache was not found so no icon is attached to it. 😢 I have no alternative in mind to make it work. |
|
@PrajeetGuha thanks for the answer, I had misunderstood that previous commit. @emtuls any ideas to find the icon? or should we merge it as it is (it needs a rebase)? |
|
We can grab the icon programmatically through a few ways, but the one I found to be easiest for me is to grab it from after it installs in this directory: There you will find We can obtain the GUID prior to installation with a small piece of code. For example (if we want to make this into a function): |



Closes: #1314