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

feat: add ssh_backup option with default true #91

Merged
merged 4 commits into from
Jun 22, 2023

Conversation

skwde
Copy link
Contributor

@skwde skwde commented Apr 26, 2023

No description provided.

@skwde skwde requested review from richm and Jakuje as code owners April 26, 2023 09:22
@Jakuje
Copy link
Collaborator

Jakuje commented Apr 26, 2023

Code-wise looks good. Would be great if we could have some test coverage for this option. You can get some inspiration from the sshd role, which has sshd_backup option and test for that:

https://github.com/willshersystems/ansible-sshd/blob/master/tests/tests_backup.yml

@richm
Copy link
Contributor

richm commented Jun 13, 2023

ping - also requires rebase

@skwde
Copy link
Contributor Author

skwde commented Jun 14, 2023

I did the rebase + added the adjusted backup tests (as in ansible-sshd). I however cannot properly test because I don't get tox-lsr running on my machine.

I would thus appreciate if somebody else could fix the remaining issues.

README.md Outdated Show resolved Hide resolved
tests/tasks/setup.yml Outdated Show resolved Hide resolved
tests/tasks/setup.yml Outdated Show resolved Hide resolved
tests/tasks/setup.yml Outdated Show resolved Hide resolved
@richm
Copy link
Contributor

richm commented Jun 14, 2023

[citest]

@richm richm changed the title add ssh_backup option with default true feat: add ssh_backup option with default true Jun 14, 2023
@richm
Copy link
Contributor

richm commented Jun 14, 2023

lgtm, except for the minor ansible-lint issues, and the Ubuntu tests

@Jakuje
Copy link
Collaborator

Jakuje commented Jun 15, 2023

I squashed the commits and added fixes/tests improvements to make the Ubuntu checks working in my branch where the CI now passes:
https://github.com/Jakuje/ssh/commits/add_ssh_backup_option
Feel free to pull the test fix commits or I can create a new PR.

@Jakuje
Copy link
Collaborator

Jakuje commented Jun 15, 2023

Please, rebase instead of merging to have nicer git history. Now the CI is green we should be good to go.

@skwde
Copy link
Contributor Author

skwde commented Jun 22, 2023

@Jakuje it should be fine now.

@Jakuje Jakuje requested a review from richm June 22, 2023 11:45
@Jakuje
Copy link
Collaborator

Jakuje commented Jun 22, 2023

Thank you! Asking also @richm for review/merge as some of the test cleanup commits was from me.

@richm richm merged commit a680fd6 into linux-system-roles:main Jun 22, 2023
11 checks passed
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

3 participants