-
Notifications
You must be signed in to change notification settings - Fork 4
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
fix(circleci): add audit, shellcheck and hadolint #447
Conversation
Codecov Report
@@ Coverage Diff @@
## master #447 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 38 38
Lines 1877 1877
Branches 342 342
=========================================
Hits 1877 1877
Continue to review full report at Codecov.
|
@linuxbandit in AEGEE/network#1 I already did some changes to the Dockerfile, but I haven't implemented those in this PR. So you can look at that as well for any optimalization |
@WikiRik apparently we also need to update the list of the required checks in order to be able to merge anything, should I do it? |
We can override it for this PR, and change it after we have merged it. I want to get Fabri's opinion on the Dockerfile first for that |
|
||
CMD sh /usr/app/scripts/bootstrap.sh && nodemon -e "js,json" lib/run.js | ||
CMD ["sh", "/usr/app/scripts/bootstrap.sh && nodemon -e 'js,json' lib/run.js"] |
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.
not sure if this'll work as there are 2 different commands
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.
Yeah, I was afraid for that already. CMD should only do 1 command though. Maybe @linuxbandit knows more on 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.
best would be to have ENTRYPOINT and CMD. I have an example on a refactoring of core dockerfile, I'll show you
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.
Splitting them is strange, so I've put a FIXME there so we can look into it later. See for more info; AEGEE/core#195 (comment)
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.
check out CMD comment, otherwise lgtm
2355471
to
4ed9e20
Compare
Changes on the lock file might look big, but because we're using |
db8d2a6
to
c3598d7
Compare
@serge1peshcoff see my comment in the conversation. We'll fix the CMD later |
Updating requires a rewrite of the notifications
## [1.6.3](1.6.2...1.6.3) (2021-02-08) ### Bug Fixes * **circleci:** add audit, shellcheck and hadolint ([#447](#447)) ([f0f9ad5](f0f9ad5))
🎉 This PR is included in version 1.6.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
With this extra bit I think I'm satisfied with the CircleCI config for a while, so I want to add this to the other modules as well after this has been merged.
@linuxbandit I added you here since I changed the Dockerfile to conform to the newest hadolint version. Please check it out to see if it is correct. If you want to do any changes to the Dockerfile to make it run quicker, please let me know and I will not merge this PR yet. I will copy these changes to the other modules as well