Skip to content

Conversation

@rlopez133
Copy link
Contributor

Adding some documentation on how to use firewalld instead of the iptables command provided in the original document.

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 19, 2018
@abhinavdahiya
Copy link
Contributor

/cc @crawford

Copy link
Member

@wking wking left a comment

Choose a reason for hiding this comment

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

And can you also squash your typo fix in, so you have a single commit?

Copy link
Member

Choose a reason for hiding this comment

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

Can you strip the trailing whitespace here (and in other places) to make Git happy:

$ git log -2 --check --oneline origin/pr/284
c66b9c9 fixed a small typo
2ce46b2 adding details regarding using firewalld instead of the iptables
Documentation/dev/libvirt-howto.md:88: trailing whitespace.
+If using `firewalld`, simply optain the name of the existing active zone which 
Documentation/dev/libvirt-howto.md:99: trailing whitespace.
+unless otherwise specified. 

Copy link
Member

Choose a reason for hiding this comment

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

The example looks like a shell session, in which case, can you add a prompt and:

```console

for highlighting? That would be:

$ sudo firewall-cmd --get-active-zones
FedoraWorkstation
  interfaces: enp0s25 tun0

Copy link
Member

Choose a reason for hiding this comment

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

You don't have any output here, and for shell examples without output we prefer no prompts and:

```sh

syntax highlighting (more on this in #27).

Copy link
Member

@wking wking left a comment

Choose a reason for hiding this comment

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

Some more minor copy-edit suggestions ;)

Copy link
Member

Choose a reason for hiding this comment

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

Can you backtick the literal --permanent?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe reword to something like:

Restart firewalld to remove your temporary changes:

or:

With the cluster removed, you no longer need to allow libvirt nodes to reach your libvirtd. Restart firewalld to remove your temporary changes:

Copy link
Member

Choose a reason for hiding this comment

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

Maybe "a simple reload of firewalld" -> firewalld --reload.

@wking
Copy link
Member

wking commented Sep 19, 2018

Ok, I think I'm done :p. This looks great, thanks :).

@rlopez133
Copy link
Contributor Author

rlopez133 commented Sep 19, 2018

Adding all let me know if its good, squashed as well

Copy link
Member

@wking wking left a comment

Choose a reason for hiding this comment

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

One nit, everything else looks great :).

Copy link
Member

Choose a reason for hiding this comment

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

With console highlighting (which I think we want because you're showing output), you need a $ prompt here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you not want prompts on the others as well?

Copy link
Member

Choose a reason for hiding this comment

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

do you not want prompts on the others as well?

Prompts belong in console blocks (to differentiate between commands and output), but not in sh blocks (because they're not part of the shell input). In this project, sh was more popular where we don't need to show output, so that's the pattern I'm trying to stick to (more on this in #27).

Copy link
Contributor

@crawford crawford left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 20, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crawford, rlopez133

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 20, 2018
@openshift-merge-robot openshift-merge-robot merged commit 32feae4 into openshift:master Sep 20, 2018
sudo firewall-cmd --zone=FedoraWorkstation --list-sources
```

NOTE: When the firewall rules are no longer needed, `firewalld --reload`
Copy link
Member

Choose a reason for hiding this comment

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

Oops, looks like this should have been firewall-cmd --reload, based on the example below :/.

wking added a commit to wking/openshift-installer that referenced this pull request Oct 11, 2018
The typo is from af6d904 (fixing cmd and typo, 2018-09-20, openshift#293),
which was itself fixing typos from 21ef0d4 (adding details regarding
using of firewalld instead of iptables, 2018-09-19, openshift#284).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants