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 Linode driver (v3 API) #1458

Merged
merged 4 commits into from Jan 18, 2019

Conversation

5 participants
@decentral1se
Copy link
Member

decentral1se commented Aug 29, 2018

This is still a work in progress. I've manually tested parts of it but not a full end to end use.

Just wanted to get this up here to see that it is all going in the right direction in case I miss something.

Will close #1450.

@retr0h

This comment has been minimized.

Copy link
Contributor

retr0h commented Aug 31, 2018

Looking good so far.... I suggest looking at #1436 and how the model was implemented along with tests. Docker and vagrant are the only drivers which really implement an API from molecule to create.yml. Might be good to define that as part of this.

@decentral1se decentral1se force-pushed the decentral1se:add-linode-driver branch from 35c4051 to d3484c0 Sep 9, 2018

decentral1se added a commit to decentral1se/molecule that referenced this pull request Sep 9, 2018

Add model testing.
As a result of direction in the following comment:

> ansible#1458 (comment)

And the good work and lead of ansible#1436 in:

> ansible#1436
@decentral1se

This comment has been minimized.

Copy link
Member Author

decentral1se commented Sep 9, 2018

I suggest looking at #1436 and how the model was implemented along with tests. Docker and vagrant are the only drivers which really implement an API from molecule to create.yml. Might be good to define that as part of this.

OK, I've added the model support in d3484c0, I think.

There are now unit tests in place to guard for a proper schema:

(.venv) ➜  molecule (add-linode-driver) ✔ py.test test/unit/model/v2/ -k linode
=============================================================== test session starts ===============================================================
platform linux2 -- Python 2.7.13, pytest-3.0.7, py-1.6.0, pluggy-0.4.0
rootdir: /home/decentral1se/hobby/molecule, inifile: pytest.ini
plugins: testinfra-1.14.1, verbose-parametrize-1.3.0, mock-1.10.0, helpers-namespace-2017.11.11, cov-2.6.0
collected 59 items 

test/unit/model/v2/test_platforms_section.py .......

=============================================================== 52 tests deselected ===============================================================
===================================================== 7 passed, 52 deselected in 0.16 seconds =====================================================
(.venv) ➜  molecule (add-linode-driver) ✔ 

Unfortunately, I've found a fairly major bug while E2E testing this which stops this feature from actually being any use at all. That is reported at ansible/ansible#45403 where the bug stops the Linode module destroying the instances it creates 🤒

I'll be heading back over that way to fix that bug. Hopefully, I can get back here in a reasonable time.

@decentral1se

This comment has been minimized.

Copy link
Member Author

decentral1se commented Sep 14, 2018

OK, false alarm on ansible/ansible#45403, it was just a docs issue (submitted ansible/ansible#45659 for that). I'll explain the short of it, since it matters here: when a linode is created a linode_id is generated and that is needed to specify that linode on deletion. So, specifying linode_id in the model doesn't make sense because it won't match the generated value, so I am going to try and adapt my destroy.yml to match what is in the ec2/destroy.yml using the molecule_instance_config lookup.

@decentral1se decentral1se force-pushed the decentral1se:add-linode-driver branch from d1e67d6 to f753d41 Sep 17, 2018

decentral1se added a commit to decentral1se/molecule that referenced this pull request Sep 17, 2018

@decentral1se decentral1se changed the title WIP: Add Linode driver Add Linode driver Sep 17, 2018

@decentral1se

This comment has been minimized.

Copy link
Member Author

decentral1se commented Sep 17, 2018

OK, @retr0h, this is ready for a review!

I see I just missed the latest release but perhaps this is better off on the devel branch for now ...

So, the manual testing I've done for this:

  • molecule init role -r foobar -d linode
  • molecule test
  • add 3 instances and run molecule test
  • molecule create && molecule login --host instance1

It seems to be in working order!

@decentral1se

This comment has been minimized.

Copy link
Member Author

decentral1se commented Sep 17, 2018

Related: #1479.

@decentral1se

This comment has been minimized.

Copy link
Member Author

decentral1se commented Sep 18, 2018

@retr0h, can I clarify if you're leaning towards rejecting this PR or accepting it?

@decentral1se decentral1se force-pushed the decentral1se:add-linode-driver branch from f753d41 to 4b9268d Sep 18, 2018

decentral1se added a commit to decentral1se/molecule that referenced this pull request Sep 18, 2018

@retr0h

This comment has been minimized.

Copy link
Contributor

retr0h commented Sep 20, 2018

@lwm Linode has been one I wanted for quite a while.

@retr0h retr0h added the v2.19 label Sep 20, 2018

@retr0h

This comment has been minimized.

Copy link
Contributor

retr0h commented Sep 20, 2018

@lwm Nicely implemented! I know adding drivers is a pain.

@retr0h

This comment has been minimized.

Copy link
Contributor

retr0h commented Sep 20, 2018

@lwm Can you please run tox -e format. Looks like formatting is off.

@decentral1se decentral1se force-pushed the decentral1se:add-linode-driver branch from 4b9268d to 19e9d89 Sep 20, 2018

decentral1se added a commit to decentral1se/molecule that referenced this pull request Sep 20, 2018

@decentral1se decentral1se force-pushed the decentral1se:add-linode-driver branch from 19e9d89 to e0ab6a7 Sep 20, 2018

decentral1se added a commit to decentral1se/molecule that referenced this pull request Sep 20, 2018

@decentral1se

This comment has been minimized.

Copy link
Member Author

decentral1se commented Sep 20, 2018

@lwm Linode has been one I wanted for quite a while. @lwm Nicely implemented! I know adding drivers is a pain.

@lwm Can you please run tox -e format. Looks like formatting is off.

Done and rebased!

@retr0h

This comment has been minimized.

Copy link
Contributor

retr0h commented Sep 20, 2018

This is missing a bunch of unit tests. I'm going through it now on my checkout. Hold off on submitting any additional work.

@decentral1se

This comment has been minimized.

Copy link
Member Author

decentral1se commented Sep 23, 2018

If it makes you feel any better, we're already using this across multiple roles on every CI run over at https://git.coop/aptivate/ansible-roles. Some logs, for example are over at https://git.coop/aptivate/ansible-roles/mysql-server/-/jobs/4071. Let me know if I can add more testing to help.

@retr0h

This comment has been minimized.

Copy link
Contributor

retr0h commented Oct 4, 2018

Sorry for the delay on this @lwm -- when I add the unit tests for the linode driver, I am getting all kinds of other problems, which means I stumbled across a bug in how I test drivers. I have been busy the last couple weeks and apologize for not looking into this sooner.

@decentral1se

This comment has been minimized.

Copy link
Member Author

decentral1se commented Oct 4, 2018

No worries! Thanks for the update. Happy to have helped smoke out a new bug :)

@gundalow

This comment has been minimized.

Copy link
Collaborator

gundalow commented Oct 8, 2018

Just in case others look at this, Linode & Molecule both have Working Groups within IRC
#ansible-linode https://github.com/ansible/community/tree/master/group-linode
#molecule-users

@retr0h

This comment has been minimized.

Copy link
Contributor

retr0h commented Oct 10, 2018

I'm going to hold off from merging this. As you know Ansible is adopting Molecule, therefore I do not want to merge anything that may impact future architectural decisions, as I will no longer be leading the development of this project.

@webknjaz

This comment has been minimized.

Copy link
Member

webknjaz commented Dec 12, 2018

@gundalow there's schema validation built-in Molecule already: https://github.com/ansible/molecule/blob/master/molecule/model/schema_v2.py

@decentral1se decentral1se referenced this pull request Jan 1, 2019

Closed

Update schema_v2.py #1639

@decentral1se

This comment has been minimized.

Copy link
Member Author

decentral1se commented Jan 1, 2019

#1639 might get me moving forward ...

@decentral1se decentral1se force-pushed the decentral1se:add-linode-driver branch from a92ca92 to 80f5fdf Jan 2, 2019

decentral1se added a commit to decentral1se/molecule that referenced this pull request Jan 2, 2019

Add Linode driver.
Closes ansible#1458.

Signed-off-by: Luke Murphy <lukewm@riseup.net>

@decentral1se decentral1se force-pushed the decentral1se:add-linode-driver branch from 80f5fdf to 70e3543 Jan 15, 2019

decentral1se added a commit to decentral1se/molecule that referenced this pull request Jan 15, 2019

Add Linode driver.
Closes ansible#1458.

Signed-off-by: Luke Murphy <lukewm@riseup.net>

@decentral1se decentral1se force-pushed the decentral1se:add-linode-driver branch 2 times, most recently from 1d3534d to d9401d3 Jan 15, 2019

@decentral1se

This comment has been minimized.

Copy link
Member Author

decentral1se commented Jan 15, 2019

OK, I'm still waiting for the build to run have seen it all go green on my local. @webknjaz, can you give this another pass? There are 3 additional commits now which aim to fix the failing tests.

@decentral1se

This comment has been minimized.

Copy link
Member Author

decentral1se commented Jan 16, 2019

@webknjaz
Copy link
Member

webknjaz left a comment

There's a few minor things to fix


.. code-block:: bash
$ pip install linode-python

This comment has been minimized.

@webknjaz

webknjaz Jan 17, 2019

Member

I think you should add it since all other drivers in master have their extras entries now.

Show resolved Hide resolved molecule/driver/linode.py Outdated
Show resolved Hide resolved test/unit/driver/test_linode.py Outdated
Show resolved Hide resolved test/unit/driver/test_linode.py Outdated
Show resolved Hide resolved test/unit/driver/test_linode.py Outdated
Show resolved Hide resolved test/unit/driver/test_linode.py Outdated
Show resolved Hide resolved test/unit/driver/test_linode.py Outdated
Show resolved Hide resolved test/unit/driver/test_linode.py Outdated
Show resolved Hide resolved test/unit/model/v2/test_platforms_section.py Outdated
Show resolved Hide resolved test/unit/model/v2/test_platforms_section.py Outdated

decentral1se and others added some commits Sep 17, 2018

Add Linode driver.
Closes #1458.

Signed-off-by: Luke Murphy <lukewm@riseup.net>
Fix incorrect copying of schema for validation.
Full credit to @mickelmayers in #1639.

Originall submitted with message like:

Platforms_base_schema destroy copy.deepcopy(base_schema). Then when
validating multiply molecules it's running with errors. Eg. two moleule
with different drivers. One Azure second docker . If docker will be
validated first from it schema we are getting. Moving
platforms_base_schema to base_schema and changing the util.merge_dicts
fixes the issue.

Closes #1639.

Signed-off-by: Luke Murphy <lukewm@riseup.net>
Merge schema only once.
This change works towards stopping schema merging clobbering themselves.
Without this change, the `test_dependency_shell_has_errors` test is
failing due to an incorrect schema.

Signed-off-by: Luke Murphy <lukewm@riseup.net>
Remove incorrect driver for fixture.
The `test_platforms_docker_has_errors` test is failing otherwise.

Signed-off-by: Luke Murphy <lukewm@riseup.net>

@decentral1se decentral1se force-pushed the decentral1se:add-linode-driver branch from 6592601 to b5dc640 Jan 17, 2019

@decentral1se

This comment has been minimized.

Copy link
Member Author

decentral1se commented Jan 17, 2019

I think you should add it since all other drivers in master have their extras entries now.

Ah, it was already added. So, fixed that documentation now as well.

I've addressed all your comments and pushed. Ready for another pass.

@decentral1se decentral1se changed the title Add Linode driver Add Linode driver (v3 API) Jan 17, 2019

@gundalow

This comment has been minimized.

Copy link
Collaborator

gundalow commented Jan 18, 2019

Thank you to everybody that's help with this!

@gundalow gundalow merged commit f09b039 into ansible:master Jan 18, 2019

@webknjaz

This comment has been minimized.

Copy link
Member

webknjaz commented Jan 18, 2019

🎉

@decentral1se decentral1se deleted the decentral1se:add-linode-driver branch Jan 18, 2019

angelbarrera92 added a commit to angelbarrera92/molecule that referenced this pull request Jan 29, 2019

Add Linode driver (v3 API) (ansible#1458)
* Add Linode driver.

Closes ansible#1458.

Signed-off-by: Luke Murphy <lukewm@riseup.net>

* Fix incorrect copying of schema for validation.

Full credit to @mickelmayers in ansible#1639.

Originall submitted with message like:

Platforms_base_schema destroy copy.deepcopy(base_schema). Then when
validating multiply molecules it's running with errors. Eg. two moleule
with different drivers. One Azure second docker . If docker will be
validated first from it schema we are getting. Moving
platforms_base_schema to base_schema and changing the util.merge_dicts
fixes the issue.

Closes ansible#1639.

Signed-off-by: Luke Murphy <lukewm@riseup.net>

* Merge schema only once.

This change works towards stopping schema merging clobbering themselves.
Without this change, the `test_dependency_shell_has_errors` test is
failing due to an incorrect schema.

Signed-off-by: Luke Murphy <lukewm@riseup.net>

* Remove incorrect driver for fixture.

The `test_platforms_docker_has_errors` test is failing otherwise.

Signed-off-by: Luke Murphy <lukewm@riseup.net>

angelbarrera92 added a commit to angelbarrera92/molecule that referenced this pull request Jan 29, 2019

Add Linode driver (v3 API) (ansible#1458)
* Add Linode driver.

Closes ansible#1458.

Signed-off-by: Luke Murphy <lukewm@riseup.net>

* Fix incorrect copying of schema for validation.

Full credit to @mickelmayers in ansible#1639.

Originall submitted with message like:

Platforms_base_schema destroy copy.deepcopy(base_schema). Then when
validating multiply molecules it's running with errors. Eg. two moleule
with different drivers. One Azure second docker . If docker will be
validated first from it schema we are getting. Moving
platforms_base_schema to base_schema and changing the util.merge_dicts
fixes the issue.

Closes ansible#1639.

Signed-off-by: Luke Murphy <lukewm@riseup.net>

* Merge schema only once.

This change works towards stopping schema merging clobbering themselves.
Without this change, the `test_dependency_shell_has_errors` test is
failing due to an incorrect schema.

Signed-off-by: Luke Murphy <lukewm@riseup.net>

* Remove incorrect driver for fixture.

The `test_platforms_docker_has_errors` test is failing otherwise.

Signed-off-by: Luke Murphy <lukewm@riseup.net>
Signed-off-by: Angel Barrera <angel.barrera@intelygenz.com>

ssbarnea added a commit to ssbarnea/molecule that referenced this pull request Jan 31, 2019

Add Linode driver (v3 API) (ansible#1458)
* Add Linode driver.

Closes ansible#1458.

Signed-off-by: Luke Murphy <lukewm@riseup.net>

* Fix incorrect copying of schema for validation.

Full credit to @mickelmayers in ansible#1639.

Originall submitted with message like:

Platforms_base_schema destroy copy.deepcopy(base_schema). Then when
validating multiply molecules it's running with errors. Eg. two moleule
with different drivers. One Azure second docker . If docker will be
validated first from it schema we are getting. Moving
platforms_base_schema to base_schema and changing the util.merge_dicts
fixes the issue.

Closes ansible#1639.

Signed-off-by: Luke Murphy <lukewm@riseup.net>

* Merge schema only once.

This change works towards stopping schema merging clobbering themselves.
Without this change, the `test_dependency_shell_has_errors` test is
failing due to an incorrect schema.

Signed-off-by: Luke Murphy <lukewm@riseup.net>

* Remove incorrect driver for fixture.

The `test_platforms_docker_has_errors` test is failing otherwise.

Signed-off-by: Luke Murphy <lukewm@riseup.net>

@decentral1se decentral1se added this to Done in release v2.20 Feb 8, 2019

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