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

Allow removal of docker container when done #1563

Merged
merged 4 commits into from
Aug 1, 2022
Merged

Allow removal of docker container when done #1563

merged 4 commits into from
Aug 1, 2022

Conversation

dmitri-trofimov
Copy link
Contributor

Fixes #1562

Proposed Changes

  1. Add a command-line option --removecontainer to the megalinter runnner
  2. The runner will pass the --rm option to docker run when the --removecontainer is specified
  3. The --rm option ensures the removal of docker container (but not the image) when linting is done.

Readiness Checklist

Author/Contributor

  • If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2022

Codecov Report

Merging #1563 (11aa9d0) into main (7acb133) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1563      +/-   ##
==========================================
+ Coverage   84.50%   84.53%   +0.02%     
==========================================
  Files         145      145              
  Lines        3562     3562              
==========================================
+ Hits         3010     3011       +1     
+ Misses        552      551       -1     
Impacted Files Coverage Δ
megalinter/reporters/UpdatedSourcesReporter.py 89.18% <0.00%> (+2.70%) ⬆️

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 7acb133...11aa9d0. Read the comment docs.

Copy link
Collaborator

@Kurt-von-Laven Kurt-von-Laven left a comment

Choose a reason for hiding this comment

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

I run docker system prune from time to time to deal with this, which works well for power users since it gives control over when the cleanup takes place, but forces additional maintenance onto the developer, and there is no guarantee that every MegaLinter user is familiar with Docker. I don’t have any particular comments on this PR besides the ones I already put on #1561, but my personal preference would be to ferry arguments through to Docker rather than mirror a subset of its API. Let us give @nvuillam a chance to weigh in before I send you off in a poor direction though.

@Kurt-von-Laven
Copy link
Collaborator

It looks good to me to merge once v6 is released!

@nvuillam
Copy link
Member

@dmitri-trofimov now v6 is released, please can you upgrade your branche by merging main into it and solve conflicts ? :)

@nvuillam
Copy link
Member

@dmitri-trofimov please could you upgrade your branch ? :)

Copy link
Member

@nvuillam nvuillam left a comment

Choose a reason for hiding this comment

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

Good, thanks for your contribution :)

@nvuillam nvuillam merged commit 75ec6f0 into oxsecurity:main Aug 1, 2022
@dmitri-trofimov dmitri-trofimov deleted the feature/remove-container-when-lint-finished branch August 1, 2022 17:06
@Kurt-von-Laven
Copy link
Collaborator

Apologies for making the same mistake twice. I just realized the argument name is likely going to trigger CSpell for our users and should be --remove-container. I can open a pull request to make the change before we release it if folks agree.

@nvuillam
Copy link
Member

nvuillam commented Aug 1, 2022

go @Kurt-von-Laven :)

@Kurt-von-Laven
Copy link
Collaborator

Went for it in #1693.

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.

Add an option to remove docker container when linting is done
4 participants