Skip to content

Added examples of options in Readme#58

Merged
richm merged 1 commit intolinux-system-roles:masterfrom
vrindle:add_examples_to_readme
Dec 14, 2021
Merged

Added examples of options in Readme#58
richm merged 1 commit intolinux-system-roles:masterfrom
vrindle:add_examples_to_readme

Conversation

@vrindle
Copy link
Collaborator

@vrindle vrindle commented Dec 6, 2021

No description provided.

@vrindle vrindle force-pushed the add_examples_to_readme branch 2 times, most recently from b8bd891 to 076b4e9 Compare December 6, 2021 09:41
set both permanent and runtime to `false`.

Examples of Options
-------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the default is permanent: yes and runtime: yes I think you can omit them from the examples, and say something like "By default, any changes will be applied immediately, and to the permanent settings. If you want the changes to apply immediately but not permanently, use permanent: no. Conversely, use runtime: no".

Permit TCP traffic for port 80 in permanent default zone:

```yaml
firewall:
Copy link
Contributor

Choose a reason for hiding this comment

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

The firewall role firewall variable value is a list - so this should be

firewall:
  - port: 80/tcp
    state: enabled

I note that there are some corrections we need to make in the role README as well - like https://github.com/linux-system-roles/firewall#state
https://github.com/linux-system-roles/firewall#example-playbooks
and in "It is also possible to combine several settings into blocks:" there are a couple of extraneous dashes - in the left hand column.

Also, the README https://github.com/linux-system-roles/firewall#variables section should start off with something like

The firewall role uses the variable `firewall` to specify the parameters.  This variable is a `list` of `dict` values.  Each `dict` value is comprised of one or more keys listed below.

@vrindle vrindle force-pushed the add_examples_to_readme branch 2 times, most recently from 16c3dac to 382f676 Compare December 13, 2021 13:44
@vrindle vrindle requested a review from richm December 13, 2021 16:41
```yaml
firewall:
- masquerade: yes
zone: dmz
Copy link
Contributor

Choose a reason for hiding this comment

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

state is a required parameter - please add state: enabled here

```yaml
firewall:
- masquerade: no
zone: dmz
Copy link
Contributor

Choose a reason for hiding this comment

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

state is a required parameter - please add state: disabled here

Copy link
Collaborator Author

@vrindle vrindle Dec 13, 2021

Choose a reason for hiding this comment

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

I think the state would have to be set to state: enabled here since masquerading cannot be used with the state being set to disabled.

module.fail_json(msg="masquerade can not be used with state: disabled")

@vrindle vrindle force-pushed the add_examples_to_readme branch from 382f676 to 6d38ee5 Compare December 13, 2021 23:06
Copy link
Contributor

@richm richm left a comment

Choose a reason for hiding this comment

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

lgtm

@richm richm merged commit 1627bc8 into linux-system-roles:master Dec 14, 2021
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.

2 participants