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

Vagrant cloud and state modules #44037

Merged
merged 7 commits into from Oct 17, 2017

Conversation

Projects
None yet
3 participants
@vernondcole
Contributor

vernondcole commented Oct 11, 2017

What does this PR do?

Provides a state module and a cloud module for Vagrant VM support.

What issues does this PR fix or reference?

Was suggested when an earlier PR with a less complete design was rejected.

Tests written?

No

vernondcole added some commits Oct 11, 2017

@garethgreenaway

This comment has been minimized.

Show comment
Hide comment
@garethgreenaway

garethgreenaway Oct 11, 2017

Member

@vernondcole Looks like you've got some lint issues here.

Member

garethgreenaway commented Oct 11, 2017

@vernondcole Looks like you've got some lint issues here.

Show outdated Hide outdated doc/topics/cloud/vagrant.rst Outdated
Show outdated Hide outdated doc/topics/cloud/vagrant.rst Outdated
Show outdated Hide outdated doc/topics/cloud/vagrant.rst Outdated
Show outdated Hide outdated doc/topics/cloud/vagrant.rst Outdated
Show outdated Hide outdated salt/cloud/clouds/vagrant.py Outdated
Show outdated Hide outdated salt/modules/vagrant.py Outdated
Possible keyword arguments:
- cwd: The directory (path) containing the Vagrantfile

This comment has been minimized.

@cachedout

cachedout Oct 11, 2017

Contributor

If these are the keyword arguments which are needed, is there a reason not to just explicitly declare them in the function interface?

@cachedout

cachedout Oct 11, 2017

Contributor

If these are the keyword arguments which are needed, is there a reason not to just explicitly declare them in the function interface?

This comment has been minimized.

@vernondcole

vernondcole Oct 12, 2017

Contributor

I think it may be better in this case to keep them generic, because I pass them around so much.

  • I hand them to _find_init_change() to parse for changes so I can report them in the ret dictionary.
  • vagrant.running uses the same set of kwargs and passes them to _find_init_change()
  • I pass them directly on to module.vagrant.init() where the defaults are defined.
  • vagrant.running also passes them to module.vagrant.init()
  • module.vagrant.init is the source of truth for the default values.
@vernondcole

vernondcole Oct 12, 2017

Contributor

I think it may be better in this case to keep them generic, because I pass them around so much.

  • I hand them to _find_init_change() to parse for changes so I can report them in the ret dictionary.
  • vagrant.running uses the same set of kwargs and passes them to _find_init_change()
  • I pass them directly on to module.vagrant.init() where the defaults are defined.
  • vagrant.running also passes them to module.vagrant.init()
  • module.vagrant.init is the source of truth for the default values.
@vernondcole

This comment has been minimized.

Show comment
Hide comment
@vernondcole

vernondcole Oct 13, 2017

Contributor

@cachedout I have implemented 6 of 7 of your comments. The seventh was phrased as a query. If you accept my answer, please accept the review. GitHub does not understand questions and treats them as requirements with a big red X. Thanks for the quality review. It really helps.

Contributor

vernondcole commented Oct 13, 2017

@cachedout I have implemented 6 of 7 of your comments. The seventh was phrased as a query. If you accept my answer, please accept the review. GitHub does not understand questions and treats them as requirements with a big red X. Thanks for the quality review. It really helps.

@cachedout

See comment wrt testing on this.

@cachedout

This comment has been minimized.

Show comment
Hide comment
@cachedout

cachedout Oct 16, 2017

Contributor

Hi @vernondcole

It doesn't look like there are any tests added here. I'm very concerned about adding an entirely new provider and module/states which are not testable. What can we do to ensure that these don't regress?

Contributor

cachedout commented Oct 16, 2017

Hi @vernondcole

It doesn't look like there are any tests added here. I'm very concerned about adding an entirely new provider and module/states which are not testable. What can we do to ensure that these don't regress?

@vernondcole

This comment has been minimized.

Show comment
Hide comment
@vernondcole

vernondcole Oct 17, 2017

Contributor

@cachedout
I agree with your concern about the lack of tests. (I am, after all, in the software Quality Assurance business.)
Unit tests will be of limited utility here -- most of this code is thin wrappers, so it would just be checking argument lists. There needs to be a few unit tests anyway, just to provide false sense of security, and to check on the parts that work a bit harder.

Mostly, I want to put in some integration tests to see that the parts play well together.

I promise, cross my heart and hope to die, to provide another Pull Request with a thorough set of tests -- as soon as I get through writing them. It will be a few weeks, since I have to finish my SaltConf presentation first.

Contributor

vernondcole commented Oct 17, 2017

@cachedout
I agree with your concern about the lack of tests. (I am, after all, in the software Quality Assurance business.)
Unit tests will be of limited utility here -- most of this code is thin wrappers, so it would just be checking argument lists. There needs to be a few unit tests anyway, just to provide false sense of security, and to check on the parts that work a bit harder.

Mostly, I want to put in some integration tests to see that the parts play well together.

I promise, cross my heart and hope to die, to provide another Pull Request with a thorough set of tests -- as soon as I get through writing them. It will be a few weeks, since I have to finish my SaltConf presentation first.

@cachedout

This comment has been minimized.

Show comment
Hide comment
@cachedout

cachedout Oct 17, 2017

Contributor

@vernondcole Since we haven't formally put the rules in place (though we will soon) regarding code coverage on the develop branch, we can get this in but I'm really, really, really hoping we do get tests in for these. These will be exceedingly hard to maintain otherwise. We'll have some changes coming to our test suite very soon which should aid in this so watch for those as well. Thanks.

Contributor

cachedout commented Oct 17, 2017

@vernondcole Since we haven't formally put the rules in place (though we will soon) regarding code coverage on the develop branch, we can get this in but I'm really, really, really hoping we do get tests in for these. These will be exceedingly hard to maintain otherwise. We'll have some changes coming to our test suite very soon which should aid in this so watch for those as well. Thanks.

@cachedout cachedout merged commit 73c3581 into saltstack:develop Oct 17, 2017

5 of 8 checks passed

GPG All commits must have a verified GPG signature
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #2921 — FAILURE
Details
codeclimate All good!
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #18413 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #11112 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #15801 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #15673 — SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment