search

linux-system-roles / nbde_client

Ansible role for configuring Network Bound Disk Encryption clients (e.g. clevis)
https://linux-system-roles.github.io/nbde_client/
MIT License
14 stars 24 forks source link

Add nbde_client role #3

Closed sergio-correia closed 4 years ago

richm commented 4 years ago

Also, please review the travis CI test failures

sergio-correia commented 4 years ago

Also, please review the travis CI test failures

Yeah, this seems more complex to get running correctly. I would appreciate help here.

sergio-correia commented 4 years ago

Thanks for the first review, @richm; I will update this shortly.

richm commented 4 years ago

Also, please review the travis CI test failures

Yeah, this seems more complex to get running correctly. I would appreciate help here.

If you just want to skip black and flake8 and pylint for now - see https://github.com/linux-system-roles/nbde_client/blob/master/.travis/config.sh - e.g. edit the file to set

export RUN_PYLINT_DISABLED=true
export RUN_BLACK_DISABLED=true
export RUN_FLAKE8_DISABLED=true

For black - if you want black to format your code - edit tox.ini like this:

commands =
#    bash {toxinidir}/.travis/runblack.sh --check --diff .
    bash {toxinidir}/.travis/runblack.sh .

then run tox -e black - black will reformat your python code to be conformant. Then revert tox.ini.

You can also review the linter issues - you can edit the various config files to completely disable that particular issue (e.g. disabling line length checking is probably the biggest one). The config.sh variables such as RUN_BLACK_EXTRA_ARGS and RUN_FLAKE8_EXTRA_ARGS can be used to pass suppressions on the command line to those programs.

You can also use code comments to suppress specific occurrences.

sergio-correia commented 4 years ago

@richm: I have removed the pytest test for now and added playbook-based tests. I will work on pytest next week. I believe this should be ready for review now.

sergio-correia commented 4 years ago

Thanks for the review @richm! I believe I have addressed all your points.

richm commented 4 years ago

ok - lgtm - @pcahyna have your concerns been addressed? looks like there is a yamllint error now

richm commented 4 years ago

got staging CI working - any idea why the Fedora tests are failing?

sergio-correia commented 4 years ago

got staging CI working - any idea why the Fedora tests are failing?

It's using the master version of the nbde_server role. The issue is probably due to the (lack of) cache update.

sergio-correia commented 4 years ago

@richm: I will add a commit changing to use the nbde_server role from the branch we are reviewing, so that we can test the CI here. I will revert the change afterwards. Is it okay?

richm commented 4 years ago

@richm: I will add a commit changing to use the nbde_server role from the branch we are reviewing, so that we can test the CI here. I will revert the change afterwards. Is it okay?

No - it isn't necessary to change to use the nbde_server from the unmerged branch, as long as we know why CI is currently failing. I guess we need to work on the nbde_server role first.

richm commented 4 years ago

Does this look good to everyone? All we need is better text for the passphrase_temporary?

pcahyna commented 4 years ago

Does this look good to everyone? All we need is better text for the passphrase_temporary?

First of all, the code and examples need changing, they are still using discard_passphrase