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

Add a Scaleway IP module #45121

Merged
merged 1 commit into from
Sep 24, 2018
Merged

Add a Scaleway IP module #45121

merged 1 commit into from
Sep 24, 2018

Conversation

remyleone
Copy link
Contributor

@remyleone remyleone commented Sep 3, 2018

SUMMARY

This PR adds a management module for Scaleway reserved IP.

Here is the list of targeted features:

  • Create a new reserved IP address
  • Attach an ip address to a server
  • Update an IP address mapping (move)
  • Remove an IP address
ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME
  • scaleway_ip
ANSIBLE VERSION
ansible 2.8.0.dev0 (scaleway_ip 7f0f872b2b) last updated 2018/09/03 17:14:39 (GMT +200)
  config file = None
  configured module search path = [u'/Users/sieben/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/sieben/workspace/ansible/lib/ansible
  executable location = /Users/sieben/workspace/ansible/bin/ansible
  python version = 2.7.15 (default, Jun 17 2018, 12:46:58) [GCC 4.2.1 Compatible Apple LLVM 9.1.0 (clang-902.0.39.2)]

@remyleone
Copy link
Contributor Author

@g1franc I started working on your dynamic IP management module. I will let you know once it is ready for review.

@ansibot
Copy link
Contributor

ansibot commented Sep 3, 2018

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

lib/ansible/modules/cloud/scaleway/scaleway_ip.py:106:18: undefined-variable Undefined variable 'volumes_json'
lib/ansible/modules/cloud/scaleway/scaleway_ip.py:114:71: undefined-variable Undefined variable 'size'
lib/ansible/modules/cloud/scaleway/scaleway_ip.py:114:92: undefined-variable Undefined variable 'volume_type'

click here for bot help

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.8 This issue/PR affects Ansible v2.8 ci_verified Changes made in this PR are causing tests to fail. cloud module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. scaleway support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. labels Sep 3, 2018
@remyleone remyleone force-pushed the scaleway_ip branch 2 times, most recently from 5625678 to 6834ee1 Compare September 4, 2018 13:56
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Sep 4, 2018
@remyleone remyleone force-pushed the scaleway_ip branch 2 times, most recently from 5e2f654 to ac52b03 Compare September 4, 2018 14:23
@ansibot
Copy link
Contributor

ansibot commented Sep 4, 2018

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

lib/ansible/modules/cloud/scaleway/scaleway_ip.py:0:0: E322 "organization" is listed in the argument_spec, but not documented in the module

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Sep 4, 2018
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Sep 4, 2018
@ansibot
Copy link
Contributor

ansibot commented Sep 5, 2018

@ansibot ansibot added the inventory Inventory category label Sep 5, 2018
@ansibot
Copy link
Contributor

ansibot commented Sep 5, 2018

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/scaleway/scaleway_ip.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/scaleway/scaleway_ip.py:0:0: missing documentation (or could not parse documentation): expected string or buffer

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/scaleway/scaleway_ip.py:0:0: missing documentation (or could not parse documentation): expected string or bytes-like object

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/cloud/scaleway/scaleway_ip.py:0:0: missing documentation (or could not parse documentation): expected string or bytes-like object

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/scaleway/scaleway_ip.py:0:0: missing documentation (or could not parse documentation): expected string or bytes-like object

The test ansible-test sanity --test compile --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/scaleway/scaleway_ip.py:158:33: SyntaxError: ip_lookup = {ip["id"]: ip for ip in ips_list}

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/scaleway/scaleway_ip.py:158:33: SyntaxError: invalid syntax

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

lib/ansible/modules/cloud/scaleway/scaleway_ip.py:0:0: E305 DOCUMENTATION.options.server.description.1: expected str @ data['options']['server']['description'][1]. Got {'To unattach an IP': "don't specify this option"}

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Sep 5, 2018
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Sep 5, 2018
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Sep 6, 2018
@g1franc
Copy link

g1franc commented Sep 7, 2018

Tested and working

@remyleone
Copy link
Contributor Author

@Spredzy @maxamillion Could you merge it?

@ansibot
Copy link
Contributor

ansibot commented Sep 15, 2018

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Sep 15, 2018
@@ -29,6 +29,13 @@

options:

dynamic_ip_required:
description:
- Enable a public IP at server creation
Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to use:

  • an hardcoded IP, for example: 163.172.152.10 (or scaleway address ID?) (IP must exist and be available in the pool of reserved IPs)
  • attached (create an IP which will be destroyed when the node will be destroyed)
  • absent (don't use any public IP)

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think this would be advantageous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Started working on it there: https://github.com/ansible/ansible/pull/44826/files

Could you review and merge it?

description:
- id of the IP

server:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not that be handled by the scaleway_compute module ?

lib/ansible/modules/cloud/scaleway/scaleway_ip.py Outdated Show resolved Hide resolved
lib/ansible/modules/cloud/scaleway/scaleway_ip.py Outdated Show resolved Hide resolved
argument_spec.update(dict(
state=dict(default='present', choices=['absent', 'present']),
organization=dict(),
server=dict(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I would let the scaleway_compute handles IP assignation to server.


id:
description:
- id of the IP
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't make sense to use id parameter with state=present. Currently when state=present is used and id is specified, a new IP with another id is created.

lib/ansible/modules/cloud/scaleway/scaleway_ip.py Outdated Show resolved Hide resolved
lib/ansible/modules/cloud/scaleway/scaleway_ip.py Outdated Show resolved Hide resolved
lib/ansible/modules/cloud/scaleway/scaleway_ip.py Outdated Show resolved Hide resolved
@remyleone remyleone force-pushed the scaleway_ip branch 2 times, most recently from 56fb225 to f21ffde Compare September 16, 2018 12:36
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Sep 16, 2018
@pilou-
Copy link
Contributor

pilou- commented Sep 20, 2018

@sieben a rebase is required for this one

@ansibot
Copy link
Contributor

ansibot commented Sep 20, 2018

@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Sep 20, 2018
@remyleone
Copy link
Contributor Author

Rebase done. I'm going to add additional tests for assigning ip to scaleway_compute

@ansibot ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Sep 21, 2018
@ansibot
Copy link
Contributor

ansibot commented Sep 21, 2018

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

lib/ansible/modules/cloud/scaleway/scaleway_compute.py:287:5: E303 too many blank lines (2)

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Sep 21, 2018
- Add an option to enable public ip at server creation
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Sep 21, 2018
@maxamillion
Copy link
Contributor

rebuild_merge

@ansibot ansibot merged commit 6d7004f into ansible:devel Sep 24, 2018
description:
- id of the Scaleway IP (UUID)

server:
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we agree that:

  • this module should not handle that
  • scaleway_compute should be used instead
    ?

Copy link
Contributor Author

@remyleone remyleone Sep 25, 2018

Choose a reason for hiding this comment

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

Indeed but I guess we can fix it in another PR.

@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 cloud core_review In order to be merged, this PR must follow the core review workflow. inventory Inventory category module This issue/PR relates to a module. new_module This PR includes a new module. new_plugin This PR includes a new plugin. scaleway support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants