-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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 additional label actions #13280
Add additional label actions #13280
Conversation
.github/label-actions.yml
Outdated
- We or the contributor doesn't have the time or equipment necessary to test it or fix it up | ||
- Sometimes the implementation isn't quite right and a different approach is necessary | ||
|
||
We would love to land this pull request when it's ready. If you have a chance to address the comments, we'd be happy to reopen and discuss merging this contribution into the framework! :tada: |
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 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.
Tiny changes, don't keel me!
Thanks for your contribution to Metasploit Framework! We've looked at this pull request, and we agree that it seems like a good addition to Metasploit, but it looks like it is not quite ready to land. We've labeled it attic
and closed it for now.
What does this generally mean? It could be one or more of several things:
- It doesn't look like there has been any activity on this pull request in a while
- We may not have the proper access or equipment to test this pull request, or the contributor doesn't have time to work on it right now.
- Sometimes the implementation isn't quite right and a different approach is necessary.
We would love to land this pull request when it's ready. If you have a chance to address all comments, we would be happy to reopen and discuss how to merge this!
|
||
This helps protect the process, ensure users are aware of commits on the branch being considered for merge, allows for a location for more commits to be offered without mingling with other contributor changes and allows contributors to make progress while a PR is still being reviewed. | ||
|
||
Please do resubmit from a unique branch, we greatly value your contribution! :tada: |
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.
At some point maybe we should enable |
# - https://github.com/rapid7/metasploit-framework/pulls | ||
``` | ||
|
||
This helps protect the process, ensure users are aware of commits on the branch being considered for merge, allows for a location for more commits to be offered without mingling with other contributor changes and allows contributors to make progress while a PR is still being reviewed. |
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've taken this from:
- update PR template to ask for unique branch #11200
- https://github.com/rapid7/metasploit-framework/wiki/So-Your-PR-was-closed#moved-to-the-attic
But I'm not quite sure I understand what it means, is there a more concise way to write it maybe? 🤔
Or it might make sense to update the wiki page with more details, and have the bot just link to the article.
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.
Related issue #11224 is referenced as documentation. It was meant to be an issue for someone-who-isn't-me to write some documentation. Ideally, the documentation could then be reference here.
- [Writing Module Documentation](https://github.com/rapid7/metasploit-framework/wiki/Writing-Module-Documentation) | ||
- [Template](https://github.com/rapid7/metasploit-framework/blob/master/documentation/modules/module_doc_template.md) | ||
- [Examples](https://github.com/rapid7/metasploit-framework/tree/master/documentation/modules) | ||
Once there's a clear path for testing and evaluating this module, we can progress with this further. |
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 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.
Small change, I'd suggest the second paragraph ("However in this case...") say instead:
We have been unable to test this module successfully. This may be due to software or hardware requirements we cannot replicate.
To help unblock this pull request, please:
etc
2927a1b
to
58641c0
Compare
rubocop -a <directory or file> | ||
``` | ||
|
||
Please update your branch after these have been made, and reach out if you have any problems. |
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 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.
Love it!
58641c0
to
71e8ce7
Compare
.github/label-actions.yml
Outdated
|
||
- Comment with links to documentation on how to set up an environment, and provide exact software version numbers to use | ||
- Or comment guided steps on how to set up our environment for testing this module | ||
- Or send pcaps/screenshots/recordings of it working and logging in as a new user - you can email us msfdev[at]rapid7.com |
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.
logging in as a new user
?
Off topic: the I noticed that this label is now being used in instances where the issue description lacks sufficient details. I suggest a separate label is created for this. Issues which demonstrate at least some effort to provide details are above potato status. |
@bcoles Thoughts on renaming |
It doesn't make much difference. I originally created the
We could probably safely auto-close issues that contain the issue template text verbatim. I forget where |
I'm fairly certain it came from t-beard, aka todb (certainly sounds like him). Absolutely not a big deal at all if it's not your preference, @adfoster-r7, but I kinda wonder whether something like |
Let's automate some more of our common workflows based on labels 🎉
Behind the scenes I will be renaming/adding labels to cover these new scenarios.