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

Merged
merged 4 commits into from
Jan 18, 2019
Merged

Conversation

decentral1se
Copy link
Contributor

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
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 added a commit to decentral1se/molecule that referenced this pull request Sep 9, 2018
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
Copy link
Contributor 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
Copy link
Contributor 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 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
Copy link
Contributor 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
Copy link
Contributor Author

Related: #1479.

@decentral1se
Copy link
Contributor Author

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

decentral1se added a commit to decentral1se/molecule that referenced this pull request Sep 18, 2018
@retr0h
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
Copy link
Contributor

retr0h commented Sep 20, 2018

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

@retr0h
Copy link
Contributor

retr0h commented Sep 20, 2018

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

decentral1se added a commit to decentral1se/molecule that referenced this pull request Sep 20, 2018
decentral1se added a commit to decentral1se/molecule that referenced this pull request Sep 20, 2018
@decentral1se
Copy link
Contributor Author

@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
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
Copy link
Contributor Author

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
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
Copy link
Contributor Author

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

@gundalow
Copy link
Contributor

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
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
Copy link
Member

webknjaz commented Dec 9, 2018

Thanks for the explanation. No ideas so far but I believe you're probably right about broken teardown: I've fixed numerous problems with it already after the acquisition and I expect that there may be more issues left unnoticed.
We'll need to tackle schema validation closer now.

@gundalow
Copy link
Contributor

Do we need something like https://github.com/ansible/ansible/blob/devel/test/sanity/validate-modules/schema.py to validate the he schema?

@webknjaz
Copy link
Member

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

@decentral1se decentral1se mentioned this pull request Jan 1, 2019
@decentral1se
Copy link
Contributor Author

#1639 might get me moving forward ...

decentral1se added a commit to decentral1se/molecule that referenced this pull request Jan 2, 2019
Closes ansible#1458.

Signed-off-by: Luke Murphy <lukewm@riseup.net>
decentral1se added a commit to decentral1se/molecule that referenced this pull request Jan 15, 2019
Closes ansible#1458.

Signed-off-by: Luke Murphy <lukewm@riseup.net>
@decentral1se decentral1se force-pushed the add-linode-driver branch 2 times, most recently from 1d3534d to d9401d3 Compare January 15, 2019 21:31
@decentral1se
Copy link
Contributor Author

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
Copy link
Contributor Author

decentral1se commented Jan 16, 2019

CI IS GREEN

EDIT: (very intense GIF) https://media0.giphy.com/media/26tPghhb310muUkEw/giphy.gif?cid=3640f6095c3f0fa370336b3655c45568)

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

There's a few minor things to fix


.. code-block:: bash

$ pip install linode-python
Copy link
Member

Choose a reason for hiding this comment

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

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

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 Show resolved Hide resolved
decentral1se and others added 4 commits January 17, 2019 23:19
Closes ansible#1458.

Signed-off-by: Luke Murphy <lukewm@riseup.net>
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>
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>
The `test_platforms_docker_has_errors` test is failing otherwise.

Signed-off-by: Luke Murphy <lukewm@riseup.net>
@decentral1se
Copy link
Contributor Author

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
Copy link
Contributor

Thank you to everybody that's help with this!

@gundalow gundalow merged commit f09b039 into ansible:master Jan 18, 2019
@webknjaz
Copy link
Member

🎉

@decentral1se decentral1se deleted the add-linode-driver branch January 18, 2019 10:47
ssbarnea pushed a commit to ssbarnea/molecule that referenced this pull request Jan 31, 2019
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Linode as a test driver
5 participants