-
Notifications
You must be signed in to change notification settings - Fork 72
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
Create template new_python_tool_pip #1232
Conversation
29a1f7c
to
689f5ed
Compare
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.
You need to add in this PR the workflow for the automation, as you need to ensure the template and the workflow work well together. For example, pkg_name
is missing and it will make it fail.
attributes: | ||
label: Tool Name | ||
description: | | ||
The name of the tool being installed (usually the file name with the `.exe`), normally different from the package name. Example: `FakeNet-NG` (tool name) vs `fakenet-ng` (package name). |
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.
There is no file with exe
in this kind of tools. Use examples of current tools installed with pip (vs tools installed in a different way). Please adapt the rest of the file to refer to Pip
tools.
Thanks @Ana06 for the review, I am addressing the changes in the template. I should not have created the PR until I finish updating the create_package_template.py and therefore test the automation. |
08edb20
to
a7b766c
Compare
@Ana06 github action runs for the automatic creation of uncompyle6 package: for rat-king-parser: |
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.
Awesome work @sara-rn! 😊 You need to add the version because (as mentioned in #1080 (comment), now tracked as #1240 that I have assigned to you) we need to fix the version of the tools installed with Pip.
$toolName = '{tool_name}' | ||
$category = '{category}' | ||
|
||
VM-Install-With-Pip -toolName $toolName -category $category |
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.
The version should be added here as well so that it is used by pip when installing the tool (otherwise you install the latest version). The template should look the same as the already existent magika package:
$version = "==0.5.0" |
e9c3d10
to
77674b3
Compare
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.
Just some nitpicks and we are ready to go! Also, please squash the commits (as the second one is adding something needed by the code in the first commit to work) and ensure to follow good commit practices such as limiting the subject line to 50 characters and using the body to explain what and why you make the changes. I recommend reading https://cbea.ms/git-commit for more info on best practices to write good commit messages. 😉
72cfe60
to
aceaf84
Compare
@Ana06 commits squashed for rat-king-parser https://github.com/sara-rn/VM-Packages/actions/runs/12813413902 |
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.
The code LGTM! 🎉 As said in my last comment: ensure to follow good commit practices such as limiting the subject line to 50 characters and using the body to explain what and why you make the changes. I recommend reading https://cbea.ms/git-commit for more info on best practices to write good commit messages. 😉
If you do not limit the subject line to 50 characters it is difficult to read, for example:
aceaf84
to
491d0d2
Compare
@Ana06 I keep forgetting about the commit message, thank you!! it's changed now |
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.
The commit message is still missing a body (description of what the change is/achieves). Let's keep it like this to avoid you have to change it again, but please take it into account for future PRs. 😉
Thanks for all the work @sara-rn , this automation enhancement is going to be very useful! @sara-rn after you merge the PR, please trigger the automation for rat-king-parser
, so exciting to see it working! 🎉
I am not sure if we can add |
yes it does but I will test it again |
Closes #1080
Create new template to automatically install Python packages with pip and update script.
The dropdown field type is mandatory in the new issue template