Add hadolint ci#115
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe update modifies the CI workflow to install and verify Hadolint, followed by linting both development and production Dockerfiles. The Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Pipeline
participant Shell as Shell Environment
participant Hadolint as Hadolint Binary
CI->>Shell: Run "Install Hadolint"
Shell->>Hadolint: Download & setup Hadolint binary
CI->>Shell: Run "Verify Hadolint installation"
Shell->>Hadolint: Display version
CI->>Shell: Run "Lint Dockerfile.dev"
Shell->>Hadolint: Lint Dockerfile.dev
CI->>Shell: Run "Lint Dockerfile.prod"
Shell->>Hadolint: Lint Dockerfile.prod
sequenceDiagram
participant Dev as Developer
participant Rake as Rake Task Runner
participant FS as File System
participant Hadolint as Hadolint Binary
Dev->>Rake: Invoke docker_lint task (with optional Dockerfile arg)
Rake->>FS: Check if specified Dockerfile exists
alt File exists
Rake->>Hadolint: Run lint on Dockerfile
Hadolint-->>Rake: Return lint results
Rake-->>Dev: Display success or error message based on results
else File missing
Rake-->>Dev: Print error and exit with status 1
end
Possibly related PRs
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: .coderabbit.yml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
sinatra/Gemfile (1)
16-16: Add gem 'rake' but maintain alphabetical order.Adding the rake gem is appropriate for supporting the new rake tasks. However, according to Ruby conventions (and the RuboCop hint), gems should be alphabetically ordered within their sections.
gem 'puma', '~> 6.6' gem 'rackup', '~> 2.2' +gem 'rake' gem 'rubocop', '~> 1.72', require: false gem 'sinatra' gem 'sinatra-contrib' gem 'sinatra-flash' gem 'sqlite3' -gem 'rake'🧰 Tools
🪛 RuboCop (1.73)
[convention] 16-16: Gems should be sorted in an alphabetical order within their section of the Gemfile. Gem
rakeshould appear beforesqlite3.(Bundler/OrderedGems)
.github/workflows/ci.yaml (2)
47-47: Remove trailing whitespace.There are trailing spaces on this line that should be removed for cleaner code.
- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 47-47: trailing spaces
(trailing-spaces)
48-67: Good implementation with some suggested improvements.The Hadolint installation and verification steps look good! A few considerations:
- Consider using the newly created Rake task rather than calling Hadolint directly in the workflow. This would ensure consistency between CLI usage and CI:
# Lint Dockerfile.dev - name: Lint Dockerfile.dev + working-directory: sinatra run: | - hadolint sinatra/Dockerfile.dev + bundle exec rake lint:dockerfile[Dockerfile.dev] # Lint Dockerfile.prod - name: Lint Dockerfile.prod + working-directory: sinatra run: | - hadolint sinatra/Dockerfile.prod + bundle exec rake lint:dockerfile[Dockerfile.prod]
- For security, consider adding checksum verification for the Hadolint download:
- name: Install Hadolint run: | + # Define the expected SHA256 checksum + EXPECTED_SHA="a54216819d5b225b4904f13f3b7710bbf2a2b71404148d8eec85d9cfe2e36b51" wget -O hadolint "https://github.com/hadolint/hadolint/releases/download/v2.12.0/hadolint-Linux-x86_64" + # Verify checksum + ACTUAL_SHA=$(sha256sum hadolint | cut -d' ' -f1) + if [ "$EXPECTED_SHA" != "$ACTUAL_SHA" ]; then + echo "Checksum verification failed!" + exit 1 + fi chmod +x hadolint sudo mv hadolint /usr/local/bin/hadolintThese changes would improve security and maintainability while keeping the original functionality intact.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
sinatra/Gemfile.lockis excluded by!**/*.locksinatra/hadolint.exeis excluded by!**/*.exe
📒 Files selected for processing (5)
.github/workflows/ci.yaml(1 hunks).gitignore(0 hunks)sinatra/Gemfile(1 hunks)sinatra/Rakefile(1 hunks)sinatra/lib/tasks/docker_lint.rake(1 hunks)
💤 Files with no reviewable changes (1)
- .gitignore
🧰 Additional context used
🪛 RuboCop (1.73)
sinatra/Gemfile
[convention] 16-16: Gems should be sorted in an alphabetical order within their section of the Gemfile. Gem rake should appear before sqlite3.
(Bundler/OrderedGems)
sinatra/lib/tasks/docker_lint.rake
[convention] 14-19: Do not use unless with else. Rewrite these with the positive case first.
(Style/UnlessElse)
🪛 YAMLlint (1.35.1)
.github/workflows/ci.yaml
[error] 47-47: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
sinatra/Rakefile (1)
1-1: LGTM: Good approach for loading Rake tasks.Dynamically loading all rake tasks from the lib/tasks directory is a clean and maintainable approach. This makes it easy to add new tasks without modifying the Rakefile.
| namespace :lint do | ||
| desc "Lint a specific Dockerfile using hadolint" | ||
| task :dockerfile, [:dockerfile] do |t, args| | ||
| # Default to 'Dockerfile.dev' if no argument is passed | ||
| dockerfile = args[:dockerfile] || "Dockerfile.dev" | ||
|
|
||
| unless File.exist?(dockerfile) | ||
| puts "#{dockerfile} not found in the project root." | ||
| exit 1 | ||
| end | ||
|
|
||
| # Run Hadolint using the local executable | ||
| puts "Running Hadolint on #{dockerfile}..." | ||
| unless system("./hadolint.exe #{dockerfile}") | ||
| puts "Hadolint found issues!" | ||
| exit 1 | ||
| else | ||
| puts "No issues found. Good job!" | ||
| end | ||
| end | ||
| end No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve platform compatibility and code style in the Docker linting task.
The Rake task functionality is good, but a few improvements would make it more robust:
-
The hardcoded "./hadolint.exe" path is Windows-specific and won't work on Linux/macOS (including GitHub Actions which runs on Ubuntu).
-
Following Ruby conventions, avoid using
unlesswithelse(as RuboCop points out).
Here's a suggested improvement:
namespace :lint do
desc "Lint a specific Dockerfile using hadolint"
task :dockerfile, [:dockerfile] do |t, args|
# Default to 'Dockerfile.dev' if no argument is passed
dockerfile = args[:dockerfile] || "Dockerfile.dev"
unless File.exist?(dockerfile)
puts "#{dockerfile} not found in the project root."
exit 1
end
# Run Hadolint using the local executable
puts "Running Hadolint on #{dockerfile}..."
- unless system("./hadolint.exe #{dockerfile}")
- puts "Hadolint found issues!"
- exit 1
- else
- puts "No issues found. Good job!"
- end
+ # Use 'hadolint' command which should be in PATH (platform-agnostic)
+ result = system("hadolint #{dockerfile}", out: $stdout, err: $stderr)
+ if result
+ puts "No issues found. Good job!"
+ else
+ puts "Hadolint found issues!"
+ exit 1
+ end
end
endAdditionally, consider capturing and displaying the actual linting output to make debugging easier.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| namespace :lint do | |
| desc "Lint a specific Dockerfile using hadolint" | |
| task :dockerfile, [:dockerfile] do |t, args| | |
| # Default to 'Dockerfile.dev' if no argument is passed | |
| dockerfile = args[:dockerfile] || "Dockerfile.dev" | |
| unless File.exist?(dockerfile) | |
| puts "#{dockerfile} not found in the project root." | |
| exit 1 | |
| end | |
| # Run Hadolint using the local executable | |
| puts "Running Hadolint on #{dockerfile}..." | |
| unless system("./hadolint.exe #{dockerfile}") | |
| puts "Hadolint found issues!" | |
| exit 1 | |
| else | |
| puts "No issues found. Good job!" | |
| end | |
| end | |
| end | |
| namespace :lint do | |
| desc "Lint a specific Dockerfile using hadolint" | |
| task :dockerfile, [:dockerfile] do |t, args| | |
| # Default to 'Dockerfile.dev' if no argument is passed | |
| dockerfile = args[:dockerfile] || "Dockerfile.dev" | |
| unless File.exist?(dockerfile) | |
| puts "#{dockerfile} not found in the project root." | |
| exit 1 | |
| end | |
| # Run Hadolint using the local executable | |
| puts "Running Hadolint on #{dockerfile}..." | |
| # Use 'hadolint' command which should be in PATH (platform-agnostic) | |
| result = system("hadolint #{dockerfile}", out: $stdout, err: $stderr) | |
| if result | |
| puts "No issues found. Good job!" | |
| else | |
| puts "Hadolint found issues!" | |
| exit 1 | |
| end | |
| end | |
| end |
🧰 Tools
🪛 RuboCop (1.73)
[convention] 14-19: Do not use unless with else. Rewrite these with the positive case first.
(Style/UnlessElse)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
sinatra/Dockerfile.prod (1)
48-50: Consolidated RUN Command for Ownership and PermissionsThe merged RUN command effectively reduces image layers by combining the change of ownership for
/appand/usr/local/bundlewith setting the executable bit fordocker-entrypoint.sh. This streamlining is beneficial for maintaining a compact image and clear command sequencing.A couple of optional suggestions:
- Validation: Ensure that the
docker-entrypoint.shscript exists as expected (although it is copied in line 47) so that thechmodoperation does not silently fail.- Security Consideration: As noted in the comments, the container currently runs as root for the sake of modifying file ownership at runtime. In a future enhancement, consider using tools like
gosuorsu-execto drop privileges as early as possible, which can further improve container security.
|
Super lækkert 👍 Jeg har lige merged dev ind i branchen for at sikre os at der ikke er nogle merge conflicts. I forhold til dette fra Coderabbit:
Den skriver hvordan det kan fixes længere oppe i denne Pull Request, men for min skyld kan vi godt lade det være, da os med macOS også bare kan skrive hadolint Dockerfile.prod/dev i vores terminal, hvis vi ønsker at køre det lokalt, så den vil jeg lade være op til dig 😊 |
|
Here's the code health analysis summary for commits Analysis Summary
|
2 participants
Description
Please provide a summary of the change and explain what problem it solves. Include any relevant motivation and context.
Related Issue:
If applicable, please reference the issue number that this pull request fixes:
Fixes #[issue-number]
Type of Change
Please check the type(s) of changes your code introduces:
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce the testing environment.
Include any details regarding your testing environment, test configuration, or steps to reproduce.
Checklist
Before creating this pull request, please ensure that your code meets the following requirements:
Summary by CodeRabbit
New Features
Chores
Refactor