Add check for non-inclusive language

[nhosoi]

Add check for non-inclusive language

Add a check for usage of terms and language that is considered non-inclusive. We are using the woke tool for this with a wordlist that can be found at
https://github.com/linux-system-roles/tox-lsr/blob/main/src/tox_lsr/config_files/woke.yml

Note: this commit uses the customized woke placed locally in .github/actions/custom-woke-action to support a new option --count-only-error-for-failure option. The local action custom-woke-action will be replaced with the official woke once get-woke/woke#252 (Add an option "--count-only-error-for-failure") is processed.

Additional note: I had difficulty ignoring the word ControlMaster at https://github.com/linux-system-roles/ssh/blob/master/.dev-tools/options_body#L22 using # wokeignore:rule=master. So, I used the customized woke to skip the warning words in this PR.

Clean up / Workaround non-inclusive words

• CHANGELOG.md
• README.md
• examples/simple.yml
• templates/ssh_config.j2
• tests/tests_global_config.yml

In addition, fixed a yamllint failure and a typo.

[richm]

lgtm - @Jakuje ok with you?

[Jakuje]

I would probably propose to wait one week more if we will get a response from the upstream about merging your changes.

There is .wokeignore which should allow you to ignore these words in the specific files (or there is ignore_files: in the config file). The files containing all configuration options would be a good candidates for such complete ignoring, or is there some issue with this approach?

The ControlMaster is used in several places in tests too and it sounds like it is poluting the code a lot. Would it be possible to adjust the configuration to ignore this for example with the word_boundary options?

[richm]

I would probably propose to wait one week more if we will get a response from the upstream about merging your changes.

ok

There is .wokeignore which should allow you to ignore these words in the specific files (or there is ignore_files: in the config file). The files containing all configuration options would be a good candidates for such complete ignoring, or is there some issue with this approach?

I would prefer not to ignore all words in a given file. If I understand .wokeignore correctly, if we add a file to this, it will ignore all words in that file. There may be some words that we do not want to ignore, especially in README.md and other highly user visible files.

But maybe this would be sufficient in .woke.yml?

- name: master
  terms:
    - master
  word_boundary: true

I think this would reject master and masters, but allow ControlMaster

but the problem here is that we are using a single .woke.yml for every role: https://github.com/linux-system-roles/ssh/pull/64/files#diff-4ceaaa93d3ad793c7dada5f77ec9b1064eb1434b545625b63d424127b0804e4fR17

and I'm not sure if we can apply this rule safely to every role
I'm also not sure if we can have multiple .woke.yml files

It would be nice if woke had the option to allow certain words, and apply the rules in order, so that we could have something like

- name: Allow ControlMaster
  terms:
    - ControlMaster
  allow: true
- name: Master
  terms:
    - master
  ....

so it would apply the rules in order, see that ControlMaster is allowed, and skip checking for master. But then this gets tricky if you have something like

Be sure to set `ControlMaster` on machines that are masters, but not slaves.

I'm hoping that woke is smart enough to scan the line for every rule.

The ControlMaster is used in several places in tests too and it sounds like it is poluting the code a lot. Would it be possible to adjust the configuration to ignore this for example with the word_boundary options?

[Jakuje]

I would prefer not to ignore all words in a given file. If I understand .wokeignore correctly, if we add a file to this, it will ignore all words in that file. There may be some words that we do not want to ignore, especially in README.md and other highly user visible files.

I meant only the files .dev-tools/options_body as it is verbatim list list of options and we can not change these, but ignoring the word or using word boundary, would be certainly better.

[richm]

I would prefer not to ignore all words in a given file. If I understand .wokeignore correctly, if we add a file to this, it will ignore all words in that file. There may be some words that we do not want to ignore, especially in README.md and other highly user visible files.

I meant only the files .dev-tools/options_body as it is verbatim list list of options and we can not change these, but ignoring the word or using word boundary, would be certainly better.

Ah, ok. Sounds good.

[nhosoi]

I would prefer not to ignore all words in a given file. If I understand .wokeignore correctly, if we add a file to this, it will ignore all words in that file. There may be some words that we do not want to ignore, especially in README.md and other highly user visible files.

I meant only the files .dev-tools/options_body as it is verbatim list list of options and we can not change these, but ignoring the word or using word boundary, would be certainly better.

What's currently done is by applying the patch get-woke/woke#252, we don't count this warning as an error (We set master as a warning, not an error). But it's reported. This is an output from tox -e woke.

.dev-tools/options_body:22:7-13: `Master` may be insensitive, use `primary`, `source`, `initiator,requester`, `controller,host`, `director` instead (warning)
ControlMaster
       ^

Do you suggest we don't apply the patch get-woke/woke#252 to the ssh role? Instead, set word_boundary: true in the central rule file .woke.yml? Thanks!

[Jakuje]

Do you suggest we don't apply the patch get-woke/woke#252 to the ssh role? Instead, set word_boundary: true in the central rule file .woke.yml? Thanks!

That would be mu suggestion as we can really not avoid these terms in the openssh configuration and in the ssh role and having the warnings shown in every pull request or run of the CI is noisy. If we could avoid it, I would prefer that way. Using word boundary was one suggestion that I noticed while running through the documentation. Using word_boundary_start: true might be even better fit, but I did not experiment with that.

[nhosoi]

Do you suggest we don't apply the patch get-woke/woke#252 to the ssh role? Instead, set word_boundary: true in the central rule file .woke.yml? Thanks!

That would be mu suggestion as we can really not avoid these terms in the openssh configuration and in the ssh role and having the warnings shown in every pull request or run of the CI is noisy. If we could avoid it, I would prefer that way. Using word boundary was one suggestion that I noticed while running through the documentation. Using word_boundary_start: true might be even better fit, but I did not experiment with that.

I understand your concerns. Here's mine... By adding boundary: true, we might miss some critical cases. Although this is not for master, I have an example. A role had files tests_sanity_something.yml. Since sanity is in the list, it was captured and fixed. If sanity has boundary: true and a new file tests_sanity_another.yml is added, it'd be skipped checking. In similar usage, we may want to capture some_master_other (across the system roles)...

[richm]

@Jakuje any further comments? Is this ok to merge now?

[Jakuje]

Looks like there was no progress in the upstream.

But I would still rather use the .wokeignore file to contain the file .dev-tools/options_body for this checking as mentioned in the first comment. Then I am not sure if we do not need the custom gh action embedded. What do you think?

[richm]

Looks like there was no progress in the upstream.

But I would still rather use the .wokeignore file to contain the file .dev-tools/options_body for this checking as mentioned in the first comment. Then I am not sure if we do not need the custom gh action embedded. What do you think?

Ok. But in the future we need to work on some improvements to woke to address situations like this - ability to specify in .wokeignore or woke.yml a specific term to allow in a specific file

[nhosoi]

Thank you for your comments, @Jakuje, @richm.
I've updated the pr to introduce .wokeignore for ControlMaster in .dev-tools/options_body and use the original woke action.

[richm]

lgtm

[Jakuje]

looks better now.
We will have to figure out how to regenerate the template to retain the woke ignore comments (or .wokeignore the template too?), but this can be handled later.

[richm]

looks better now. We will have to figure out how to regenerate the template to retain the woke ignore comments (or .wokeignore the template too?), but this can be handled later.

Yeah, I think we should .wokeignore the template - otherwise, the next time a parameter changes and we need to regenerate the template, woke is going to complain again. And I think that's more likely than fixing woke to avoid this situation, at least in the near future.

[nhosoi]

Sorry, is the "template" in "we should .wokeignore the template" the generated config file templates/ssh_config.j2? Currently, ControlMaster in ssh_config.j2 is ignored by the comment {# wokeignore:rule=master #}. Instead, we should just skip checking ssh_config.j2 completely?

[richm]

Sorry, is the "template" in "we should .wokeignore the template" the generated config file templates/ssh_config.j2?

Yes. This file is generated by .dev-tools/make_option_list using the files in .dev-tools/. We should not have to generate it, then edit it by hand to add the wokeignore directives.

Currently, ControlMaster in ssh_config.j2 is ignored by the comment {# wokeignore:rule=master #}. Instead, we should just skip checking ssh_config.j2 completely?

Yes. Then in the future we should work on woke to make it work better for our use cases.