-
Notifications
You must be signed in to change notification settings - Fork 10
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
first semgrep sarif codemod for jinja autoescape #687
Conversation
dc06953
to
e585cf7
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.
Looks great, only substantive comment is about the codemod ID.
from core_codemods.semgrep.api import SemgrepCodemod | ||
|
||
SemgrepEnableJinja2Autoescape = SemgrepCodemod.from_core_codemod( | ||
name="enable-jinja2-autoescape-semgrep", |
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 don't think we need semgrep
in the codemod name since its encoded by the origin.
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're right! Since now only core codemods can be requested by name. The Sonar ones have the id in the name which hinted at that when I wrote this.
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.
@clavedeluna I think the new test failure is real since it's asserting on the previous name 😅
911bb32
to
0266d9f
Compare
nvm fixed |
@drdavella sorry for the churn here, this is the first time we have a codemod with the same name as another so it's causing more test failure and questions than I expected. The latest failure suggests something new, which is that we can't register a codemod with the same name as another here. It just gets skipped. We have two options here
Both are easy enough changes I can do it here. LMK what you think works best in terms of customer usability / backward compat. I personally would just go for #2 and nuke the idea of codemod name if we can, to avoid future confusion. edit: I guess another option just to get this PR thru is to just add -semgrep again to the name and then ticket this for later. |
@clavedeluna I like solution #1 for the short term if it gets us over the hump for this PR. I think we were implicitly doing that anyway (or at least that was my personal assumption). #2 might not be a bad idea to ticket for the longer term. |
offline we've agreed to just add -semgrep to the name and work on the options mentioned above in a separate ticket |
Quality Gate passedIssues Measures |
See #697 |
Overview
Add support for OSS semgrep codemod for jinja2 autoescape, along with the necessary code to support semgrep codemods in general
Closes #677