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

Refactor format_allowed_section in gce_net module. #20814

Open
wants to merge 4 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@peterandiw

peterandiw commented Jan 30, 2017

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

cloud/google/gce_net.py

ANSIBLE VERSION
ansible 2.2.1.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = Default w/o overrides
SUMMARY

This is to increase understandability of the code, so hopefully the flow
can help those who read this code to better understand its output and
to prevent possibility of future bugs.

Being related to fix #20528, this is a refactor to some of the code beginning from 3b667b6's commit for better understanding.

Refactor format_allowed_section in gce_net module.
This is to increase understandability of the code, so hopefully the flow
can help those who read this code to better understand its output.
@ansibot

This comment has been minimized.

Contributor

ansibot commented Jan 30, 2017

@supertom

This comment has been minimized.

Contributor

supertom commented Jan 30, 2017

Hi @peterandiw, thanks for having a look at this. I agree this code is convoluted.

Would you mind some unit tests in test/units/modules/cloud/google/test_gce_net.py (you'll need to create this file)? Adding the tests will allow everyone to keep this function maintained. Furthermore, we're planning a refactoring of the entire module, and so, having tests for standalone functions like this will allow them to be easily ported over.

@ansibot ansibot removed the needs_triage label Jan 30, 2017

@peterandiw

This comment has been minimized.

peterandiw commented Feb 3, 2017

Sure @supertom . Sorry it took me quite a long time to respond.

peterandiw added some commits Feb 3, 2017

Replace assertDictEqual with assertEqual.
(?for backward compatibility?)
@peterandiw

This comment has been minimized.

peterandiw commented Feb 3, 2017

Considering the latest test result, I think I need to refactor the other method first.

@mattclay

This comment has been minimized.

Member

mattclay commented Feb 3, 2017

@mattclay mattclay added the ci_verified label Feb 3, 2017

@ansibot ansibot added the stale_ci label Apr 11, 2017

@ansibot

This comment has been minimized.

Contributor

ansibot commented Jun 24, 2017

@ansibot

This comment has been minimized.

Contributor

ansibot commented Jan 6, 2018

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