-
Notifications
You must be signed in to change notification settings - Fork 33
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
Include sast check for dockerfiles #696
Conversation
This reverts commit 11402cf.
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.
I'm a little curious. Are these checks recommended ones?
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.
Yes, from the security team. I can share with you internally the link if you are interested, Aki.
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.
Yes, please. I should have known them before I write any dockerfile :-)
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.
Np, they seem relatively new. I didn't know them until we started some weeks ago with the deployment.
Just trying to think through the workflow here... if this test is only run on push to main, does that mean the flow would be something like this:
If that's right, if the docker verify test fails, then the PR author would have to make changes but the PR would still be approved... if the changes needed to pass a security review were non-trivial, I could see wanting another review before merging... |
Not exactly, the workflow works too in a PR everytime in the PR exists a change in the specified path. In #689 it was working because to pass the checks I needed to do changes in the Dockerfiles. How at this time we don't need to fix anything the workflow doesn't run. |
Now @psschwei you should be able to see the checks with the last change that I did, for example. |
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.
LGTM. Our Dockerfiles pass these checks for now (with some warning) :-). It's good.
I don't know why this error didn't appear in #699 but I will take a look now 😂 |
Seems a bit random this error 🤷♂️ |
Summary
This PR includes checks that validates our Dockerfiles configurations.
Details and comments
conftest
ref #690