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

cloudscale_server: refactor to implement update #52683

Merged
merged 17 commits into from Mar 5, 2019

Conversation

Projects
None yet
3 participants
@resmo
Copy link
Member

commented Feb 20, 2019

SUMMARY

Refactored to implement the update functionality, it has been tested with exiting tests before adding the new tests covering the updating parts. Further the refactored module has --diff support implemented.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

cloudscale_server

ADDITIONAL INFORMATION

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

Show resolved Hide resolved lib/ansible/modules/cloud/cloudscale/cloudscale_server.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/cloudscale/cloudscale_server.py Outdated

@resmo resmo force-pushed the resmo:feature/cloudscale_server_update2 branch Feb 21, 2019

Show resolved Hide resolved lib/ansible/module_utils/cloudscale.py Outdated

@resmo resmo force-pushed the resmo:feature/cloudscale_server_update2 branch Feb 22, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/cloud/cloudscale/cloudscale_server.py:0:0: E307 version_added should be 2.3. Currently '2.3'

click here for bot help

@resmo

This comment has been minimized.

Copy link
Member Author

commented Feb 22, 2019

waiting for #52806 to fix the false negative sanity failure

@resmo

This comment has been minimized.

Copy link
Member Author

commented Feb 22, 2019

ready_for_review

@gaudenz
Copy link
Contributor

left a comment

Thanks for your work! It's nice to see this module being improved. Please also have a look at my suggestion regarding the code you already fixed since I started my review.

Show resolved Hide resolved lib/ansible/module_utils/cloudscale.py Outdated
Show resolved Hide resolved lib/ansible/module_utils/cloudscale.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/cloudscale/cloudscale_server.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/cloudscale/cloudscale_server.py
Show resolved Hide resolved lib/ansible/modules/cloud/cloudscale/cloudscale_server.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/cloudscale/cloudscale_server.py Outdated
Show resolved Hide resolved lib/ansible/module_utils/cloudscale.py Outdated
Show resolved Hide resolved test/integration/targets/cloudscale_server/tasks/main.yml
Show resolved Hide resolved test/integration/targets/cloudscale_server/tasks/main.yml
Show resolved Hide resolved test/integration/targets/cloudscale_server/tasks/main.yml

@ansibot ansibot removed the needs_triage label Feb 25, 2019

@ansibot ansibot removed the needs_rebase label Feb 28, 2019

@resmo resmo changed the title cloudscale_server: refactor to implement update WIP: cloudscale_server: refactor to implement update Feb 28, 2019

@resmo

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

I addressed the points of the review, clarifying docs is open yet. However, the update method has a lot of duplicate logic and looks over complicated, I am refactoring to simplify it.

@ansibot ansibot added the WIP label Feb 28, 2019

resmo added some commits Feb 28, 2019

@resmo resmo changed the title WIP: cloudscale_server: refactor to implement update cloudscale_server: refactor to implement update Feb 28, 2019

@resmo

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

ready_for_review

@gaudenz
Copy link
Contributor

left a comment

Thanks for all the changes. There is only one issue from my review left that I would like to discuss:

For the case where the flavor changes I'm not sure if I like the warning or if I'd prefer the module to fail. IMO modules should fail when they can't do a requested action. Or is there an Ansible policy that this should only be a warning?

@resmo

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

I don't like the idea to fail. It is not an error, it is just not clear if the user does agree to take the server offline to change it. Hence the force param and a message to the user.

(Hypothetical, if we would fail, shouldn't we then also fail on other params which the api doesn't allow to update?)

Last but not least there are other cloud modules using the same behavior.

@resmo

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

ready_for_review

@gaudenz

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

Thanks for the last small change and the explanation.

shipit

@ansibot ansibot added shipit and removed community_review labels Mar 5, 2019

@resmo resmo merged commit aafc553 into ansible:devel Mar 5, 2019

1 check passed

Shippable Run 111761 status is SUCCESS.
Details

@resmo resmo deleted the resmo:feature/cloudscale_server_update2 branch Mar 5, 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.