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 support for 'privileged' search #19120
base: master
Are you sure you want to change the base?
Conversation
Hi @nrathaus thanks for all the recent contributions 🚀 I thought I'd add a few things we like to do/see when contributing. It's super helpful for everyone involved if contributors follow the pull request template. Just a brief description of what the changes are and why they were needed, replication and verification steps for the testing you completed when verifying the change. This is will help us work towards making sure all bases are covered in terms of testing path, and with the project being a long running open source project, it is also super helpful for future traveler's when changes are well documented with a good paper trail to reference back to. |
Isn't linking to the original Issue sufficient here? @cgranleese-r7 |
Co-authored-by: cgranleese-r7 <69522014+cgranleese-r7@users.noreply.github.com>
Co-authored-by: cgranleese-r7 <69522014+cgranleese-r7@users.noreply.github.com>
@cgranleese-r7 how can I get the modules_metadata updated with the privileged attribute? Is there a way to populate the JSON file? |
No, due to the points I have listed above it is much more beneficial to make use of our template.
If I'm understanding what you're asking, I believe that will be done automatically once the PR is merged. |
@cgranleese-r7 The 'privileged' field information is read from the |
Thanks for the PR! I think the code changes in isolation are good, but I think the #
# Returns whether or not the module requires or grants high privileges.
#
def privileged?
privileged == true
end Because of this ambiguity in definition, i.e. the module either requiring or granting privileges, I'm not sure if exposing these values to the user is beneficial in its current state I'm wondering if we might need to do some pre-requisite work to split this one value into two? i.e. |
|
Resolves #18990