[Github] Security impact field in templates #14666
Conversation
There was a problem hiding this comment.
Looks good! Noticed one problem with the Issue template, but aside from that, my only feedback is:
I don't know if it makes the most sense to me to have both Issues and PRs capture this information. Issues can get outdated somewhat easily if the nature of the work being done ends up changing in the process of doing it. Also, there are some cases where it makes sense to have multiple PRs per Issue, where each might have different security implications. Since cases exist where it only makes sense to put this information in the PR description, and I'm not sure there are cases where it only makes sense to put it in an Issue, and for the sake of consistency, I would prefer to just use the PR description. Happy to be overruled if the rest of the team finds having the option to put it in Issues helpful, though!
Also this is definitely outside the scope of this PR, but maybe we could create a custom GitHub Action (similar to this) to check that at least one of these boxes is checked off before allowing a merge, so it's not just up to the reviewers to catch that.
| label: Security Impact | ||
| description: Level of security impact of the change | ||
| options: | ||
| - None |
There was a problem hiding this comment.
When I try to preview this template, I get this error about None being a reserved keyword, since it is the default option when a dropdown field is not marked as required.
I think it would make the most sense to make this field required and change this option to be called No impact, so that when filling out the template, we have to explicitly make a choice about what level of impact it will have. However, if we'd prefer to keep it as an optional field, we can just remove this option, and None will still be present in the dropdown (and will be the default option).
|
@iris-garden I think you make great points! And I agree, across many PRs we probably do want to be analyzing the security impacts at every stage, not just as a one-off "when we're done it will be X" analysis in the ticket... So I guess in my mind the only real reason for using the issue-level review would be for tracking the impact of non-code changes (like configuration updates to production). I will try to make the templates reflect that distinction |
…, made PR security assessment required and added some examples.
For feedback - a couple of potential templates for capturing security impacts at either the issue or PR level.