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

Exclude packages that have state set to absent. #409

Merged
merged 1 commit into from
Dec 12, 2018
Merged

Exclude packages that have state set to absent. #409

merged 1 commit into from
Dec 12, 2018

Conversation

robertdebock
Copy link
Contributor

Signed-off-by: Robert de Bock robert@meinit.nl

This PR solved #405 by excluding tasks that have state set to absent. Tests are included in pass locally using tox.

Signed-off-by: Robert de Bock <robert@meinit.nl>
# Exclude tasks where state is `absent`.
if task['action'].get('state') not in self._module_ignore_states:
message = '{0} {1}'
return message.format(self.shortdesc, module)
Copy link
Member

Choose a reason for hiding this comment

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

How about

Suggested change
return message.format(self.shortdesc, module)
return ' '.join((self.shortdesc, module))

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

  1. Unrelated changes shouldn't be mixed (minor concern)
  2. Methods/functions must return the same predictable type of data (important)
  3. I'm deeply allergic to nesting (sorry, couldn't pass by)
Don't unfold this :)


# Exclude tasks where state is `absent`.
if task['action'].get('state') not in self._module_ignore_states:
message = '{0} {1}'
return message.format(self.shortdesc, module)
Copy link
Member

Choose a reason for hiding this comment

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

Oh, the return value should be bool. It's strongly discouraged to mix different return types in a function.

Copy link
Contributor

Choose a reason for hiding this comment

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

For better or worse, currently ansible-lint rules use mixed return types, returning True ansible-lint cli outputs the shortdesc field, returning text ansible-lint cli outputs the text. In this case True might be descriptive enough for this rule.

@@ -18,7 +18,7 @@ class PackageHasRetryRule(AnsibleLintRule):
# | sort | awk -F '/' \
# '/__|dpkg|_repo|_facts|_sub|_chan/{next} {split($NF, words, ".");
# print "\""words[1]"\","}'
package_modules = [
_package_modules = [
Copy link
Member

Choose a reason for hiding this comment

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

Why do you think this change is related to the PR purpose? I'd not want to see non-related changes making PR non-atomic and addressing multiple things.

if module in self._package_modules:
if 'until' not in task:
# Exclude tasks where state is `absent`.
if task['action'].get('state') not in self._module_ignore_states:
Copy link
Member

Choose a reason for hiding this comment

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

It's 3 nesting levels. Better use "guard expressions" style to keep it flat.

return (task['action']['__ansible_module__'] in self.package_modules
and 'until' not in task)
module = task["action"]["__ansible_module__"]
if module in self._package_modules:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if module in self._package_modules:
if module not in self._package_modules or 'until' in task:
return False

will help keep 2 indentation levels less and will hugely improve readability.

if module in self._package_modules:
if 'until' not in task:
# Exclude tasks where state is `absent`.
if task['action'].get('state') not in self._module_ignore_states:
Copy link
Member

Choose a reason for hiding this comment

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

This will collapse to

Suggested change
if task['action'].get('state') not in self._module_ignore_states:
return task['action'].get('state') not in self._module_ignore_states

after the suggestions above.

return (task['action']['__ansible_module__'] in self.package_modules
and 'until' not in task)
module = task["action"]["__ansible_module__"]
if module in self._package_modules:
Copy link
Member

Choose a reason for hiding this comment

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

Another approach to improve readability would be to have "named conditions":

Suggested change
if module in self._package_modules:
is_module_package = module in self._package_modules
is_task_retryable = 'until' in task
is_state_whitelisted = task['action'].get('state') in self._module_ignore_states
return is_module_package and not is_task_retryable and is_state_whitelisted

Flat and simple.

@awcrosby
Copy link
Contributor

awcrosby commented Dec 7, 2018

I think these are good edits and we should get them in for 4.0.0. I agree with the indenting suggestions.

@awcrosby awcrosby added this to the 4.0.0 milestone Dec 7, 2018
@awcrosby awcrosby merged commit 04ce914 into ansible:master Dec 12, 2018
@awcrosby
Copy link
Contributor

Merging this as-is, I’ll address the suggestions in a separate PR. Thanks for the contribution @robertdebock!

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