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 tags parameter to packet_device #418

Conversation

akiselev1
Copy link
Contributor

SUMMARY

packet_device module currently does not handle tags in device creation. Tags are supported by packet.net api. This change is intended to fill this gap.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

packet_device

ADDITIONAL INFORMATION

Before the change:

Creating devices

  • name: Create 1 device
    hosts: localhost
    tasks:
    • packet_device:
      project_id: 89b497ee-5afc-420a-8fb5-56984898f4df
      hostnames: myserver
      operating_system: ubuntu_16_04
      plan: baremetal_0
      facility: sjc1

After the change:

Creating devices

  • name: Create 1 device
    hosts: localhost
    tasks:
    • packet_device:
      project_id: 89b497ee-5afc-420a-8fb5-56984898f4df
      hostnames: myserver
      tags: ci-xyz
      operating_system: ubuntu_16_04
      plan: baremetal_0
      facility: sjc1

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added affects_2.10 feature This issue/PR relates to a feature request module module new_contributor Help guide this first time contributor labels May 26, 2020
@ansibullbot
Copy link
Collaborator

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

plugins/modules/cloud/packet/packet_device.py:431:1: E302: expected 2 blank lines, found 1
plugins/modules/cloud/packet/packet_device.py:444:1: E302: expected 2 blank lines, found 1

click here for bot help

@ansibullbot ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label May 26, 2020
@akiselev1 akiselev1 force-pushed the ansible-collection-packet-tags branch from cc062cd to 3d98135 Compare May 26, 2020 22:03
@ansibullbot ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label May 26, 2020
Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the PR
We also need a changelog fragment https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs-how-to

minor_changes:
- packet_device - add ``tags`` parameter (https://github.com/ansible-collections/community.general/pull/418).

plugins/modules/cloud/packet/packet_device.py Outdated Show resolved Hide resolved
plugins/modules/cloud/packet/packet_device.py Outdated Show resolved Hide resolved
@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels May 27, 2020
@akiselev1 akiselev1 force-pushed the ansible-collection-packet-tags branch from 3d98135 to bfe69f6 Compare May 27, 2020 15:09
@akiselev1 akiselev1 changed the title Add tags to packet device creation Add tags parameter to packet_device May 27, 2020
@ansibullbot
Copy link
Collaborator

The test ansible-test sanity --test ansible-doc [explain] failed with the error:

Command "ansible-doc -t module community.general.packet_device" returned exit status 1.
>>> Standard Error
ERROR! module community.general.packet_device missing documentation (or could not parse documentation): while parsing a block collection
  in "<unicode string>", line 40, column 7
did not find expected '-' indicator
  in "<unicode string>", line 41, column 7

The test ansible-test sanity --test validate-modules [explain] failed with 10 errors:

plugins/modules/cloud/packet/packet_device.py:0:0: deprecation-mismatch: "meta/runtime.yml" and DOCUMENTATION.deprecation do not agree.
plugins/modules/cloud/packet/packet_device.py:0:0: doc-choices-do-not-match-spec: Argument 'state' in argument_spec defines choices as (['absent', 'active', 'inactive', 'rebooted', 'present']) but documentation defines choices as ([])
plugins/modules/cloud/packet/packet_device.py:0:0: doc-choices-do-not-match-spec: Argument 'wait_for_public_IPv' in argument_spec defines choices as ([4, 6]) but documentation defines choices as ([])
plugins/modules/cloud/packet/packet_device.py:0:0: doc-default-does-not-match-spec: Argument 'count' in argument_spec defines default as (1) but documentation defines default as (None)
plugins/modules/cloud/packet/packet_device.py:0:0: doc-default-does-not-match-spec: Argument 'count_offset' in argument_spec defines default as (1) but documentation defines default as (None)
plugins/modules/cloud/packet/packet_device.py:0:0: doc-default-does-not-match-spec: Argument 'state' in argument_spec defines default as ('present') but documentation defines default as (None)
plugins/modules/cloud/packet/packet_device.py:0:0: doc-default-does-not-match-spec: Argument 'wait_timeout' in argument_spec defines default as (900) but documentation defines default as (None)
plugins/modules/cloud/packet/packet_device.py:0:0: doc-elements-mismatch: Argument 'tags' in argument_spec specifies elements as str,but elements is not documented
plugins/modules/cloud/packet/packet_device.py:0:0: doc-required-mismatch: Argument 'project_id' in argument_spec is required, but is not documented as being required
plugins/modules/cloud/packet/packet_device.py:52:7: documentation-syntax-error: DOCUMENTATION is not valid YAML

The test ansible-test sanity --test yamllint [explain] failed with 2 errors:

plugins/modules/cloud/packet/packet_device.py:40:7: unparsable-with-libyaml: while parsing a block collection - did not find expected '-' indicator
plugins/modules/cloud/packet/packet_device.py:52:7: error: DOCUMENTATION: syntax error: expected <block end>, but found '?' (syntax)

click here for bot help

@Andersson007
Copy link
Contributor

@baldwinSPC @nurfet-becirevic @t0mk @teebes as authors could you look at the changes please?

Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

Let's wait for the authors a couple of days. If nobody's against, we'll merge it as it is

@ansibullbot ansibullbot added community_review and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels May 27, 2020
@akiselev1
Copy link
Contributor Author

@Andersson007 I pinged them on slack. Keeping my fingers crossed 🙂

@gianarb
Copy link

gianarb commented May 27, 2020

Hello!!
Thank you for doing it! I work at Packet but I am far away from being an Ansible expert/user. I had a look at the code and it sounds good, it interfaces well with the PacketAPI.
I am not sure about how to get up and running with this version of the module to try it out by myself.
This is the Vagrantfile I put together:

# -*- mode: ruby -*-
# vi: set ft=ruby :
Vagrant.configure("2") do |config|
  config.vm.box = "bento/ubuntu-20.04"
  config.vm.provider "virtualbox" do |vb|
    vb.memory = "1024"
  end
  config.vm.provision "shell", inline: <<-SHELL
    apt-get update
    apt install -y software-properties-common
    apt-add-repository --yes --update ppa:ansible/ansible
    apt install -y ansible python3-pip
    pip3 install packet-python
  SHELL
end

How can I checkout this branch? Thanks

Response

As suggested by @akiselev1 this made the trick:

# Checkout the fork and commit you want on /home/vagrant/community.general
export ANSIBLE_LIBRARY=~/.ansible/plugins/modules/:/home/vagrant/community.general/plugins/modules/

And I got my awesome server provisioned and tagged as needed! For me, it is good to go.

About the tag limit, from the documentation there is nothing official about it, I am trying to get some more information about it but I think in the meantime we can leave it out and if the number sounds so weird that requires a pre-check we can add it later!

Thanks a lot!

@akiselev1 akiselev1 force-pushed the ansible-collection-packet-tags branch from 9e7e81e to a89f68e Compare May 27, 2020 17:05
@felixfontein
Copy link
Collaborator

BTW, this describes how you can properly check out a collection git repo and work with it: https://docs.ansible.com/ansible/devel/dev_guide/developing_collections.html#contributing-to-collections

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

What I don't understand is how this PR does tag management. If you specify a tag that is not there for a present machine, or remove a tag that is there: how does it make sure that the machine afterwards has the new set of tags? Or does it only sets tags on creation? If that's the case, this must be clearly documented.

plugins/modules/cloud/packet/packet_device.py Outdated Show resolved Hide resolved
@akiselev1 akiselev1 force-pushed the ansible-collection-packet-tags branch from a89f68e to 26a5bcc Compare May 28, 2020 16:46
@akiselev1
Copy link
Contributor Author

@felixfontein Thanks for review. Great questions! My current goal was to add device tag(s) on device creation only. I updated tags description and the changelog with this statement. Let me know if this clarification is needed anywhere else.

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Looks good to me (without having any clue what packet does ;) ).

@gianarb @andfasano do I understand correctly that you are happy with the current state of the PR? If you don't write otherwise until say mid next week, I'll merge.

@felixfontein
Copy link
Collaborator

(@akiselev1 please ping me if I forget :) )

Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@gianarb gianarb left a comment

Choose a reason for hiding this comment

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

LGTM

@andfasano
Copy link

LGTM

@Andersson007 Andersson007 merged commit 317532f into ansible-collections:master May 29, 2020
@Andersson007
Copy link
Contributor

@akiselev1 thanks for the contribution!
@felixfontein @gianarb @andfasano thanks for reviewing!

@Andersson007
Copy link
Contributor

merged #418 into master

@akiselev1 akiselev1 deleted the ansible-collection-packet-tags branch May 29, 2020 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request module module new_contributor Help guide this first time contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants