-
-
Notifications
You must be signed in to change notification settings - Fork 0
feat(docker): general docker improvements #16
Conversation
Reviewer's GuideThis PR standardizes container build artifacts by renaming Dockerfiles to Containerfiles and docker-compose manifests to compose files, updates CI workflows and linting targets accordingly, applies formatting fixes via linters, simplifies the networking configuration in compose, and updates related documentation. Class diagram for CI workflow job renaming and logic changesclassDiagram
class CIWorkflow {
+containerfile-lint
+compose-lint
+security-scan
}
CIWorkflow : -dockerfile-lint
CIWorkflow : -docker-compose-lint
CIWorkflow : +containerfile-lint
CIWorkflow : +compose-lint
CIWorkflow : updated file detection logic
CIWorkflow : updated build and lint targets
CIWorkflow : updated output messages
CIWorkflow : updated security scan to use Containerfile
CIWorkflow : updated compose file validation
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
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.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `.github/workflows/ci.yml:118` </location>
<code_context>
uses: hadolint/[email protected]
with:
- dockerfile: './Dockerfile'
+ dockerfile: './Containerfile'
failure-threshold: warning
format: sarif
</code_context>
<issue_to_address>
Linting is now hardcoded to './Containerfile', which may miss Containerfiles in subdirectories.
This will result in only the root Containerfile being linted. To ensure all Containerfiles are checked, iterate over all relevant files.
Suggested implementation:
```
- name: Find all Containerfiles
id: find_containerfiles
run: |
find . -type f \( -name 'Containerfile' -o -name 'Dockerfile' \) > containerfile_list.txt
- name: Lint all Containerfiles with Hadolint (Security Report)
if: steps.containerfiles.outputs.found == 'true'
run: |
while IFS= read -r file; do
hadolint "$file" --format sarif >> hadolint-results.sarif || true
done < containerfile_list.txt
```
- Ensure that Hadolint is available in the runner environment (install if needed).
- If you want to keep using the Hadolint GitHub Action, you may need to use a matrix strategy or a custom script instead, as the action does not natively support multiple files.
- The aggregation of SARIF results may require post-processing if you want a single SARIF file for upload.
- Adjust the `Upload Hadolint results to GitHub Security` step to use the correct output file path if changed.
</issue_to_address>
### Comment 2
<location> `.github/workflows/ci.yml:189` </location>
<code_context>
- # Find all docker-compose files in the repository
- compose_files=$(find . -name "docker-compose*.yml" -o -name "docker-compose*.yaml" -o -name "compose*.yml" -o -name "compose*.yaml" | grep -v ".git")
+ # Find all compose files in the repository
+ compose_files=$(find . -name "compose*.yml" -o -name "compose*.yaml" -o -name "compose*.yml" -o -name "compose*.yaml" | grep -v ".git")
if [ -n "$compose_files" ]; then
echo "found=true" >> $GITHUB_OUTPUT
</code_context>
<issue_to_address>
Duplicate file patterns in compose file discovery.
The file patterns '-name "compose*.yml"' and '-name "compose*.yaml"' are listed twice in the find command. Removing the duplicates will make the code clearer.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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, ready to merge?
Previous tests were reported as 100% successful aside from one skipped test. I think this can be merged, I just want to double check that all tests pass... and it did.
0c1dc79 to
0da191c
Compare
|
LGTM |
Summary by Sourcery
Rename Docker build and compose files to Containerfile and compose.yml, modernize linting workflows, reformat compose configuration, and simplify Docker networking
New Features:
Enhancements:
CI:
Documentation: