Skip to content
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

CI: Add hadolint validation via GitHub workflow #3761

Closed
wants to merge 1 commit into from

Conversation

kalikiana
Copy link
Member

Using hadolint via GHA, locally tested like so:

podman run --rm -i hadolint/hadolint <container/worker/Dockerfile
podman run --rm -i hadolint/hadolint <container/webui/Dockerfile

See: poo#88451

Comment on lines 11 to 15
dockerfile: './container/webui/Dockerfile'
- uses: brpaz/hadolint-action@v1.2.1
with:
dockerfile: './container/worker/Dockerfile'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess using our own shell style call for this could help to catch all Dockerfiles, not just the ones that are explicitly specified

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is #3 about this, so basically we could run hadolint ./container/**/Dockerfile with container: hadolint/hadolint. Upstream seems happy to take fixes, not sure how easy that is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but what I am suggesting is to not rely on github action specific stuff that people can not reproduce locally and also is not complete but rather use simple shell scripts that solve both points, same as in https://github.com/os-autoinst/os-autoinst/pull/1604/files#diff-1af57dc1e7a5a58067566f2787f0030953af77313ae8d5488b06f71a851640b7R11

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a totally different point. But yes, I also brought it up in a daily last week, where these points came up:

  • You can see the reproducer in the PR description but by default GHAs don't advertise how to reproduce them locally.
  • GHAs are nice and concise snippets which we don't have to maintain.
  • Usually makefile targets are the most convenient to reproduce something locally.

So maybe what we want here is a test-containers-hadolint target, and test-containers-compose for #3758 respectively.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, not as straightforward as I thought, I'm getting all kinds of funny errors trying to simply translate the glob to something running in the container... but added the new target with two individual lines anyway

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have seen and understood the line
https://github.com/os-autoinst/os-autoinst/pull/1604/files#diff-1af57dc1e7a5a58067566f2787f0030953af77313ae8d5488b06f71a851640b7R11
?

Maybe you forgot to pass the current working directory into the container because the globbing works on passing paths to hadolint within the container so hadolint needs to be able to find the Dockerfile's within the container environment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seen yes, probably not understood. I get "command not found" type errors with different variations and I have no idea how the current working directory relates to the command I'm running. I assume the escaping is also different.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is more obvious to you, I'd welcome a one-line suggestion to make it work in the Makefile.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variants including -v $(pwd):/root --workdir=/root and $(readlink -f "$(dirname "$0")"/..):/root --workdir=/root

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I consider the best approach doing the same as in os-autoinst, so trying with #3772

@codecov
Copy link

codecov bot commented Mar 3, 2021

Codecov Report

Merging #3761 (f2bd7ac) into master (0b1531c) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3761      +/-   ##
==========================================
- Coverage   96.49%   96.44%   -0.05%     
==========================================
  Files         368      368              
  Lines       32438    32432       -6     
==========================================
- Hits        31302    31280      -22     
- Misses       1136     1152      +16     
Impacted Files Coverage Δ
t/lib/OpenQA/Test/Database.pm 84.48% <0.00%> (-6.90%) ⬇️
t/lib/OpenQA/Test/Utils.pm 77.74% <0.00%> (-3.76%) ⬇️
lib/OpenQA/Shared/Controller/Auth.pm 98.00% <0.00%> (-0.14%) ⬇️
t/ui/01-list.t 99.04% <0.00%> (-0.01%) ⬇️
t/ui/07-file.t 100.00% <0.00%> (ø)
t/ui/26-jobs_restart.t 100.00% <0.00%> (ø)
t/36-job_group_defaults.t 100.00% <0.00%> (ø)
t/39-scheduled_products-table.t 100.00% <0.00%> (ø)
t/ui/27-plugin_obs_rsync_status_details.t 85.45% <0.00%> (ø)
lib/OpenQA/Scheduler/Model/Jobs.pm 97.66% <0.00%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b1531c...85a3f01. Read the comment docs.

@kalikiana kalikiana force-pushed the ci_hadolint branch 2 times, most recently from 2271ab9 to 8aab657 Compare March 8, 2021 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants