-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: Comment Audit result on PR (only on errors) #384
base: staging
Are you sure you want to change the base?
feat: Comment Audit result on PR (only on errors) #384
Conversation
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 good to me, thank you @eduardo-getpassport! I have only three minor notes below that you can ignore if you’d like 😉
path: . | ||
if-no-files-found: error | ||
retention-days: 1 | ||
if: steps.audit-packages.outputs.exit_code == 1 |
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 wonder if steps.audit-packages.conclusion == 'failure'
is more reliable because any non-zero exit code is considered a failure (and we check only for 1
*)?
—————
* At first glance, 1
seems to be the only exit code that pip-audit
returns.
@behnazh I’m actually surprised that referencing workflow steps works across different workflows (src) 🤔 I wasn’t able to find that documented, so hopefully that handy feature isn’t a bug. For example, in if: steps.audit-packages.outputs.exit_code == 1 which references the |
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.
Thanks, looks great! But the pipelines are failing. I think the problem is storing the result file at the root directory, which flit
is not tracking and therefore it complains. You can either store it at dist/
or /tmp
?
Good point. I had missed it. Actually I doubt it works and will probably fail at runtime. @eduardo-getpassport have you tested these workflows? |
Yes! I just tested again. I commented on the After that I commenter the PR ones and works. I think since we are using the This is the first run with only the build commented: Link The second run with the if commented in both files: Link PS: Only on my runs, I commented the Mac and Windows jobs since we are expensive 😱 |
Fixed on separate branch, pushing the changes rn |
@eduardo-getpassport I'm trying to debug this. The |
With PR #551 in mind I wonder if it would make sense to post an informative message on a PR if package auditing is disabled? |
What's on this PR
pr-change-set.yaml
:build
step and post a comment with the output.build.yaml
:continue-on-error: true
inAudit installed packages
step . In case of an audit error, the next step will runaudit-packages
inAudit installed packages
step. This is to reference this step in the later one.run
command to export the output.What
Comment vulnerabilities on PR
step does:Takes the file generated in the previous step in case of an error and posts the content inside the PR.
Example: