-
Notifications
You must be signed in to change notification settings - Fork 79
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
Implement GitHub Actions workflow for scanning Docker images (Ref: #301) #317
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.
Thanks for working on this @Hayyaaf ! LGTM -- and I like the approach of being able to run this only manually: Nice to the environment, not running these scan-builds always/unnecessarily. Might it a good improvement to also run it on push-to-main as a safeguard (either in this job or as an additional step in the main build-and-push jobs we already have)?
Signed-off-by: Khalid <187553667+Hayyaaf@users.noreply.github.com> Add push-to-main trigger
e66af3e
to
f4a55d0
Compare
Thanks for the feedback, @baentsch. I have updated the workflow in the same file to include a trigger for push-to-main. Let me know if there’s anything else you’d like adjusted. |
Nope -- that's OK for the time being. I'd like to see the new workflow become available and we can then improve in followup PRs, e.g. while doing #294, adding this scan logic at the end of the build(s) to avoid CI run duplication. Thanks for this contribution! |
for FILE in $FILES; do | ||
IMAGE_NAME="${{ matrix.folder }}-$(basename $FILE | tr '[:upper:]' '[:lower:]' | tr -cd '[:alnum:]-')" | ||
echo "Building Docker image: $IMAGE_NAME using $FILE" | ||
docker build -t $IMAGE_NAME -f $FILE ./${{ matrix.folder }} |
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.
This logic has a flaw @Hawazyn : If finding a Dockerfile in a subdirectory (such as nginx/fulltest), the subsequent docker build
command does not switch CWD to that subdirectory, leading to files not being found. IMO this is the reason for the persistent CI failure, e.g., here. Please let me know whether you'd provide a PR to fix for this or whether I should do to get CI status back to green.
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 @baentsch, I’ll look into this issue today and provide a fix PR.
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.
On closer look, I'm wondering whether this can be improved in general: Am I right thinking that this job duplicates the tasks in the other CI workflows (building images)? Wouldn't it be better in general to only add a scan step to each separate image build as and when done anyway? This would be much less resource intensive. OK for you to change things this way? This would also do away with this separate file entirely (and thus automatically solve the problem above): OK for you @Hawazyn ?
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 agree with combining the scan with image builds for efficiency. Should I proceed to apply the changes and remove the docker-scan file?
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.
If you have time for that, that would be welcome.
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’ll work on this, but I kindly ask for some additional time as I am currently managing other priorities, in addition to work related to Docker files and their documentation. I truly appreciate your patience and understanding, and I will keep you updated as I make progress.
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.
No urgency -- would you mind if I did it to save you the hassle and let you focus on what you're already doing?
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.
Thank you for offering to help! I can see you already have a great deal to manage at the moment, and while I am perfectly happy to continue with this, I do not wish to add to your workload unnecessarily. My tasks are quite manageable, so please let me know how you would prefer to proceed. I am happy to adjust to whatever suits you best.
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 sincerely apologize for being unable to address the workflow scanning integration as planned due to unforeseen circumstances. Unfortunately, I will be unavailable for the next month and unable to take on new tasks or make additional changes during this time. However, I am committed to finalizing PR #338 to ensure the current work is completed before my absence.
If there are further issues or adjustments needed, I kindly request your assistance to ensure a smooth resolution. I deeply appreciate your understanding and support.
This pull request enhances the GitHub Actions workflow to streamline Docker image scanning, addressing issue #301.
Key Updates
workflow_dispatch
.Successfully tested on a forked repository.
Reference: #301