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

sysctl: Rename ignoreerrors to ignore_unknown_keys #49253

Open
wants to merge 3 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@winem

winem commented Nov 28, 2018

This PR renames ignoreerrors to ignore_errors and keeps backward compatibility with ignorerrors.

It's basically to be consistent with all other modules.

The sysctl module is the only one where the attribute is called ignoreerrors and not ignore_errors.
The new parameter is called ignore_errors but supports ignoreerrors as an alias.
The alias ensures that backward compatibility is given and this is not a breaking change.

SUMMARY

With this PR ignore_errors is allowed, too. I found it a bit unintuitive because it is not consistent with all other modules where the parameter is called ignore_errors with an underscore. The alias is to ensure backward compatibility because this cosmetically change should not break any playbooks out there.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

sysctl

ADDITIONAL INFORMATION

Documentation was updated and 2 tests to test both, ignoreerrors and ignore_errors were added.

Marcel Weinberg
Rename ignoreerrors to ignore_errors
This PR is basically to be consistent with all other modules.

The sysctl module is the only one where the attribute is called ignoreerrors and not ignore_errors.

The new parameter is called ignore_errors but supports ignoreerrors as an alias.

The alias ensures that backward compatibility is given and this is not a breaking change.
@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 28, 2018

Hi @winem, thank you for submitting this pull-request!

click here for bot help

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 28, 2018

@bcoca

modules don't implement ignore_errors, it is a keyword of the task, this will lead to confusion, i almost prefer keeping the old name to disambiguate.

@winem

This comment has been minimized.

winem commented Nov 28, 2018

@bcoca I totally agree here. It's a bit confusing especially when you combine both or use both in the same playbooks. To be honest, it even took me a moment to understand the different effects of ignoreerrors and ignore_errors.

So I took another look at the other modules and it's really just the keyword that it's referenced there.

I have mixed feelings about this PR and am not sure if consistency is worth the risk for confusion here. I let this up to you, guys. I also don't mind to change anything here.

(Will take a look at the failing tests for Fedora, CentOS, etc. tomorrow. Already 3 a.m. here.)

@winem

This comment has been minimized.

winem commented Nov 29, 2018

I thought a bit more about this and, if at all, it probably just makes sense to choose another name for the parameter (i.e. ignore_unknown_keys). ignore_errors on 2 different levels is something that should be avoided.

The reason why I changed my opinion a bit is this from the previous comment: "So I took another look at the other modules and it's really just the keyword that it's referenced there."

If there is no further input or request from anyone else with an opinion on this toic, I'll close this PR.

@winem

This comment has been minimized.

winem commented Dec 3, 2018

@bcoca what do you think about the option to rename it to "ignore_unknown_keys"? I think that this is a small improvement and not as ambiguous as ignoreerrors (although the documentation is unambiguous here.
So this is more for people just reading a playbook without having all the modules and parameters in mind.

@gundalow

This comment has been minimized.

Contributor

gundalow commented Dec 3, 2018

ignore_unknown_keys sounds good

@winem

This comment has been minimized.

winem commented Dec 4, 2018

Ok, will provide an updated PR.

@gundalow gundalow changed the title from Rename ignoreerrors to ignore_errors to sysctl: Rename ignoreerrors to ignore_unknown_keys Dec 4, 2018

Marcel Weinberg
sysctl - rename ignoreerrors to ingore_unknown_keys
ignore_unknown_keys is not as ambiguous as ignorerrors
ignoreerrors could be confused with the task keyword ignore_errors
and ignore_unknown_keys is more specific about in regards to what it actually does
@bcoca

bcoca approved these changes Dec 5, 2018

@mattclay

This comment has been minimized.

Member

mattclay commented Dec 5, 2018

Some of our test containers don't support reloading. Using reload: no as is done on the other tasks will work. Is there a reason why that can't be done for the new tests?

@bcoca

This comment has been minimized.

Member

bcoca commented Dec 5, 2018

@mattclay can we make it conditional? it would be nice to also test reloading .. since it is an integral part of the plugin

@winem

This comment has been minimized.

winem commented Dec 6, 2018

I'm all in for conditionals on the conditional reload. Just let me know the OS or rather OS family (or any other condition) for containers that do not support reload. Any details are appreciated. @mattclay

@ansibot ansibot added the stale_ci label Dec 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment