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

Ufw to obj #54708

Open
wants to merge 5 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@jbarotin
Copy link
Contributor

jbarotin commented Apr 2, 2019

SUMMARY

I need this refactoring that transform ufw module from functional to object. Internal state will be better handled (with the mandatory self prefix)

ISSUE TYPE
  • Refactoring Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Apr 2, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Apr 2, 2019

The test ansible-test sanity --test pylint [explain] failed with 7 errors:

lib/ansible/modules/system/ufw.py:347:11: undefined-variable Undefined variable 'params'
lib/ansible/modules/system/ufw.py:348:30: undefined-variable Undefined variable 'params'
lib/ansible/modules/system/ufw.py:350:28: undefined-variable Undefined variable 'params'
lib/ansible/modules/system/ufw.py:352:41: undefined-variable Undefined variable 'module'
lib/ansible/modules/system/ufw.py:352:61: undefined-variable Undefined variable 'ufw_bin'
lib/ansible/modules/system/ufw.py:367:28: undefined-variable Undefined variable 'params'
lib/ansible/modules/system/ufw.py:409:4: no-self-argument Method should have "self" as first argument

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Apr 2, 2019

@jbarotin jbarotin force-pushed the jbarotin:ufw_to_obj branch Apr 2, 2019

jbarotin added some commits Feb 4, 2019

@jbarotin jbarotin force-pushed the jbarotin:ufw_to_obj branch to 33488b6 Apr 2, 2019

@ansibot ansibot added core_review and removed needs_revision labels Apr 2, 2019

@jbarotin

This comment has been minimized.

Copy link
Contributor Author

jbarotin commented Apr 2, 2019

Hi @felixfontein what do you think about that PR ? I need it to set up #52029

elif self.pre_rules != rules_dry:
self.__setChanged()

def __setChanged(self):

This comment has been minimized.

Copy link
@felixfontein

felixfontein Apr 3, 2019

Contributor

Is there a reason you use two underscores here (and not for any other method)? Also, does this really needs to be its own method? (Same question for is_check_mode, which could also be done by setting self.check_mode = module.check_mode in __init__ and then using self.check_mode instead of self.is_check_mode().)

params = module.params
def ufw_default(self, value, direction):

# A TESTER : le type est défini avec choice donc une autre valeur n'est pas posible (TEST U ?)

This comment has been minimized.

Copy link
@felixfontein

felixfontein Apr 3, 2019

Contributor

All comments should be in English.

@ansibot ansibot removed the needs_triage label Apr 3, 2019

@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Apr 3, 2019

@jbarotin What are your plans regarding handling state, resp. how do you plan to use this PR to implement diff mode?

@jbarotin

This comment has been minimized.

Copy link
Contributor Author

jbarotin commented Apr 3, 2019

@felixfontein thanks for the review, I'll take time to fix your comment. Yes, I need this PR to implement the diff mode. As it is a refactoring, I sent it separately in a first step.

@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Apr 3, 2019

If you already have part 2 (i.e. the diff mode implementation), can you create another PR for it (including the commits from this PR), marked as [WIP], so that it is easier to see where this is going?

Also, you could please take a look at #54799? We apparently missed that when default is specified, direction does not needs to be specified...

@jbarotin

This comment has been minimized.

Copy link
Contributor Author

jbarotin commented Apr 3, 2019

@felixfontein Yep I will do that (the WIP PR)

@ansibot ansibot added the stale_ci label Apr 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.