Add support for additional flags #53

mjourdan · 2022-05-13T14:31:40Z

🗣 Description

💭 Motivation and context

Wanted to automatically move the infected files to a quarantine folder. Also, allow some tuning.

This PR resolves #51 and #50.

🧪 Testing

I ran the role against a Debian 11 target host.

- hosts: all
  become: yes
  become_method: sudo
  vars:
    clamav_scan_move: true
  roles:
    - clamav

✅ Pre-approval checklist

This PR has an informative and human-readable title.
Changes are limited to a single goal - eschew scope creep!
All relevant type-of-change labels have been added.
I have read the CONTRIBUTING document.
These code changes follow cisagov code standards.
All relevant repo and/or project documentation has been updated to reflect the changes in this PR.
Tests have been added and/or modified to cover the changes in this PR.
All new and existing tests pass.

dav3r

@mjourdan Thanks very much for contributing to this repo! Please take a look at my suggestions and let me know if you have any questions or concerns.

README.md

defaults/main.yml

tasks/main.yml

templates/virus_scan.sh

dav3r

Looks great to me now - thank you @mjourdan for contributing!

@jsf9k, @mcdonnnj, or @felddy - can you please give this PR a once-over?

mcdonnnj

Thanks for your contribution! This all looks solid. I have some minor changes I'd like to see below.

README.md

defaults/main.yml

tasks/main.yml

templates/virus_scan.tpl.sh

jsf9k

I'd like to see some additional tests that cover the new options you have added. It may make sense to create some new molecule scenarios for this, as is done, e.g., in cisagov/ansible-role-python#46.

README.md

defaults/main.yml

ClementJ35 · 2022-10-14T13:48:39Z

Hi @jsf9k,
I did the tests as you did for modify_config. It works nice (see https://github.com/mjourdan/ansible-role-clamav/actions/runs/3250333690 )
Here, it can't run because of merge conflict.
What is your policy ?
Do we merge / rebase / do nothing ?

Fixes cisagov#51

fixes cisagov#50

Alphabetize, clean up wording, and fix typos/formatting Co-authored-by: dav3r <daver@geekpad.com>

Alphabetize Co-authored-by: dav3r <daver@geekpad.com>

Co-authored-by: dav3r <daver@geekpad.com>

avoid abbreviation Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>

replace dict with list Co-authored-by: Shane Frasier <maverick@maverickdolphin.com>

remove unnecessary quotes on file mode Co-authored-by: Shane Frasier <maverick@maverickdolphin.com>

Add backticks to default values in readme Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>

jsf9k · 2022-10-17T14:58:37Z

Hi @jsf9k, I did the tests as you did for modify_config. It works nice (see https://github.com/mjourdan/ansible-role-clamav/actions/runs/3250333690 ) Here, it can't run because of merge conflict. What is your policy ? Do we merge / rebase / do nothing ?

Thanks @ClementJ35 and @mjourdan! I rebased the PR; once the tests pass I'll re-review.

jsf9k · 2022-10-17T15:38:04Z

The build is failing because of an issue in molecule. Hopefully that issue is resolved upstream soon.

ClementJ35 · 2022-10-19T09:29:20Z

Hi @jsf9k, thanks for rebasing.
It seems your PR as been merged : ansible/molecule#3687
I'll keep an eye here to see if it's passing all tests.

jsf9k · 2022-10-19T13:25:32Z

Hi @jsf9k, thanks for rebasing. It seems your PR as been merged : ansible-community/molecule#3687 I'll keep an eye here to see if it's passing all tests.

Yep, once they release a new version of Molecule I can re-run the tests and they should pass.

jsf9k

Great work @mjourdan! Thank you for the contribution!

jsf9k · 2022-10-26T20:40:20Z

@dav3r and @mcdonnnj, please re-review this pull request.

dav3r

Looks great - strong work! 🚀

mcdonnnj

Thanks for the continued work in getting this implemented. I'm suggesting that the additional_flags scenario's files more closely mirror the form of the modify_config scenario so that we are consistent. Other than that all I saw was a typo.

README.md

molecule/additional_flags/INSTALL.rst

molecule/additional_flags/prepare.yml

molecule/additional_flags/python.yml

molecule/additional_flags/requirements.yml

molecule/additional_flags/upgrade.yml

molecule/additional_flags/molecule.yml

molecule/additional_flags/molecule-no-systemd.yml

molecule/additional_flags/molecule-with-systemd.yml

Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>

This entailed making several files in molecule/additional_flags symlinks instead of hard copies. I also broke the tests that were in default/tests/test_default.py into two files so I could reuse most of them in molecule/additional_flags but still create a slightly different version of others specific to the scenario. Co-authored-by: Nicholas McDonnell <50747025+mcdonnnj@users.noreply.github.com>

dav3r

Still good, aside from one missed docstring update. 🚀

molecule/additional_flags/tests/test_files_and_dirs_additional_flags.py

Co-authored-by: dav3r <daver@geekpad.com>

mcdonnnj

Just a small docstring fix from me.

molecule/additional_flags/tests/test_additional_flags.py

Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>

jsf9k · 2022-11-03T03:14:59Z

@mjourdan and @ClementJ35, thanks for the contribution and for sticking with us. This one is finally merged!

mjourdan · 2022-11-07T09:17:05Z

Thank you @jsf9k @dav3r @mcdonnnj @ClementJ35 \o/

mjourdan requested review from dav3r, felddy and jsf9k as code owners May 13, 2022 14:31

dav3r requested changes May 18, 2022

View reviewed changes

dav3r approved these changes May 27, 2022

View reviewed changes

mcdonnnj added the improvement This issue or pull request will add or improve functionality, maintainability, or ease of use label May 27, 2022

mcdonnnj assigned jsf9k and dav3r May 27, 2022

mcdonnnj requested changes May 28, 2022

View reviewed changes

jsf9k requested changes May 31, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

defaults/main.yml Outdated Show resolved Hide resolved

defaults/main.yml Outdated Show resolved Hide resolved

Mathieu Jourdan and others added 19 commits October 17, 2022 10:35

Turn cron job from simple file to template

ab0f4ba

Add support for quarantine and extra flags

fc99731

Fixes cisagov#51

replace exclude dir strings with regular expressions

c261fc6

fixes cisagov#50

add quarantine dir to the list of exclusions

27cca4c

Update README.md

967e67c

Alphabetize, clean up wording, and fix typos/formatting Co-authored-by: dav3r <daver@geekpad.com>

Update defaults/main.yml

b5f0f27

Alphabetize Co-authored-by: dav3r <daver@geekpad.com>

Rename template to match cisagov standard

8e56031

Update tasks/main.yml

f0155c1

Co-authored-by: dav3r <daver@geekpad.com>

alphabetize clamscan flags

9fc64dd

Apply suggestions from code review

724a795

avoid abbreviation Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>

Update README.md

3678c9a

replace dict with list Co-authored-by: Shane Frasier <maverick@maverickdolphin.com>

Update defaults/main.yml

8d5d28a

remove unnecessary quotes on file mode Co-authored-by: Shane Frasier <maverick@maverickdolphin.com>

remove unnecessary quotes on quarantine directory

9d09781

Apply suggestions from code review

7f8704a

Add backticks to default values in readme Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>

[tests] First commit of tree folder

9d643a8

[tests] Second commit of the new folder tree

fcaffab

[tests] Write first test

e2c16d8

[test] add comment and fix syntax

0feb478

[tests] Fix lint & add the scenarios to the matrix

bb91af9

ClementJ35 added 2 commits October 17, 2022 10:56

[tests] handle line break in string declaration

7e968c7

[tests] Fix dummy switches order in content_test

8225ff0

jsf9k force-pushed the feat_additional_flags branch from 6cadc30 to 8225ff0 Compare October 17, 2022 14:56

jsf9k added 2 commits October 26, 2022 15:38

Update test code to match long CLI option in virus_scan script

834e909

Modify test to make it less brittle

f9b98be

jsf9k requested review from dav3r, mcdonnnj and jsf9k October 26, 2022 20:36

jsf9k approved these changes Oct 26, 2022

View reviewed changes

jsf9k unassigned dav3r Oct 26, 2022

dav3r approved these changes Oct 27, 2022

View reviewed changes

mcdonnnj approved these changes Nov 2, 2022

View reviewed changes

jsf9k and others added 2 commits November 2, 2022 12:26

Fix typo

391c1b2

Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>

jsf9k force-pushed the feat_additional_flags branch from 2e1b063 to 9713cb1 Compare November 2, 2022 17:57

jsf9k requested review from dav3r and mcdonnnj November 2, 2022 18:07

dav3r approved these changes Nov 2, 2022

View reviewed changes

molecule/additional_flags/tests/test_files_and_dirs_additional_flags.py Outdated Show resolved Hide resolved

Update docstring to indicate the correct scenario

d4c39eb

Co-authored-by: dav3r <daver@geekpad.com>

mcdonnnj approved these changes Nov 2, 2022

View reviewed changes

molecule/additional_flags/tests/test_additional_flags.py Outdated Show resolved Hide resolved

Add an underscore in a scenario name in a docstring

2635b49

Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>

jsf9k merged commit f0756e3 into cisagov:develop Nov 3, 2022

jsf9k mentioned this pull request Nov 17, 2023

Clamd Permissions Fix for Debian AppArmor #74

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for additional flags #53

Add support for additional flags #53

mjourdan commented May 13, 2022 •

edited by jsf9k

Loading

dav3r left a comment

dav3r left a comment

mcdonnnj left a comment

jsf9k left a comment

ClementJ35 commented Oct 14, 2022 •

edited

Loading

jsf9k commented Oct 17, 2022

jsf9k commented Oct 17, 2022 •

edited

Loading

ClementJ35 commented Oct 19, 2022

jsf9k commented Oct 19, 2022

jsf9k left a comment

jsf9k commented Oct 26, 2022

dav3r left a comment

mcdonnnj left a comment

dav3r left a comment

mcdonnnj left a comment

jsf9k commented Nov 3, 2022

mjourdan commented Nov 7, 2022

Add support for additional flags #53

Add support for additional flags #53

Conversation

mjourdan commented May 13, 2022 • edited by jsf9k Loading

🗣 Description

💭 Motivation and context

🧪 Testing

✅ Pre-approval checklist

dav3r left a comment

Choose a reason for hiding this comment

dav3r left a comment

Choose a reason for hiding this comment

mcdonnnj left a comment

Choose a reason for hiding this comment

jsf9k left a comment

Choose a reason for hiding this comment

ClementJ35 commented Oct 14, 2022 • edited Loading

jsf9k commented Oct 17, 2022

jsf9k commented Oct 17, 2022 • edited Loading

ClementJ35 commented Oct 19, 2022

jsf9k commented Oct 19, 2022

jsf9k left a comment

Choose a reason for hiding this comment

jsf9k commented Oct 26, 2022

dav3r left a comment

Choose a reason for hiding this comment

mcdonnnj left a comment

Choose a reason for hiding this comment

dav3r left a comment

Choose a reason for hiding this comment

mcdonnnj left a comment

Choose a reason for hiding this comment

jsf9k commented Nov 3, 2022

mjourdan commented Nov 7, 2022

mjourdan commented May 13, 2022 •

edited by jsf9k

Loading

ClementJ35 commented Oct 14, 2022 •

edited

Loading

jsf9k commented Oct 17, 2022 •

edited

Loading