From ebc57b978ff71923fc015fe6c8f513d8d9003d01 Mon Sep 17 00:00:00 2001 From: ffa Date: Wed, 31 Oct 2012 21:05:47 -0700 Subject: [PATCH 1/5] Demo of saltstack/salt#2417 module standards choice "1" I left a note in "salt/modules/git.py" for the _git_run method. For "salt/states/git.py", the two public methods illustrate the two different methods utilizing the proposed module standards in a salt state. * "git.latest" uses "try[..]catch" blocks to return the error from the module as part of the state's return output. * errors from modules called in "git.present" pass through to the minion which "[...]itself does catch all published executions exceptions and returns based on that information" [1] As you will see, implementing the standard is quick, easy, simple, and non invasive. In fact, most of my time was spent writing this commit message. [1] thatch45, https://github.com/saltstack/salt/issues/2417#issuecomment-9969939 --- salt/modules/git.py | 42 ++++++++++++++++++++++------------- salt/states/git.py | 53 +++++++++++++++++++++++++-------------------- 2 files changed, 56 insertions(+), 39 deletions(-) diff --git a/salt/modules/git.py b/salt/modules/git.py index 06b2d5cd82..ddb6eb1b42 100644 --- a/salt/modules/git.py +++ b/salt/modules/git.py @@ -5,6 +5,23 @@ Support for the Git SCM import os from salt import utils +def _git_run(**kwargs): + ''' + simple, throw an exception with the error message on an error return code. + + this function may be moved to the command module, spliced with + 'cmd.run_all', and used as an alternative to 'cmd.run_all'. Some + commands don't return proper retcodes, so this can't replace 'cmd.run_all'. + ''' + result = __salt__['cmd.run_all'](**kwargs) + + retcode = result['retcode'] + + if retcode == 0: + return result['stdout'] + else: + raise Exception(result['stderr']) + def _git_getdir(cwd, user=None): ''' Returns the absolute path to the top-level of a given repo because some Git @@ -45,12 +62,7 @@ def revision(cwd, rev='HEAD', short=False, user=None): _check_git() cmd = 'git rev-parse {0}{1}'.format('--short ' if short else '', rev) - result = __salt__['cmd.run_all'](cmd, cwd, runas=user) - - if result['retcode'] == 0: - return result['stdout'] - else: - return '' + return _git_run(cmd, cwd, runas=user) def clone(cwd, repository, opts=None, user=None): ''' @@ -82,7 +94,7 @@ def clone(cwd, repository, opts=None, user=None): opts = '' cmd = 'git clone {0} {1} {2}'.format(repository, cwd, opts) - return __salt__['cmd.run'](cmd, runas=user) + return _git_run(cmd, runas=user) def describe(cwd, rev='HEAD', user=None): ''' @@ -146,7 +158,7 @@ def archive(cwd, output, rev='HEAD', fmt=None, prefix=None, user=None): fmt = ' --format={0}'.format(fmt) if fmt else '', prefix = ' --prefix="{0}"'.format(prefix if prefix else basename)) - return __salt__['cmd.run'](cmd, cwd=cwd, runas=user) + return _git_run(cmd, cwd=cwd, runas=user) def fetch(cwd, opts=None, user=None): ''' @@ -173,7 +185,7 @@ def fetch(cwd, opts=None, user=None): opts = '' cmd = 'git fetch {0}'.format(opts) - return __salt__['cmd.run'](cmd, cwd=cwd, runas=user) + return _git_run(cmd, cwd=cwd, runas=user) def pull(cwd, opts=None, user=None): ''' @@ -196,7 +208,7 @@ def pull(cwd, opts=None, user=None): if not opts: opts = '' - return __salt__['cmd.run']('git pull {0}'.format(opts), cwd=cwd, runas=user) + return _git_run('git pull {0}'.format(opts), cwd=cwd, runas=user) def rebase(cwd, rev='master', opts=None, user=None): ''' @@ -224,7 +236,7 @@ def rebase(cwd, rev='master', opts=None, user=None): if not opts: opts = '' - return __salt__['cmd.run']('git rebase {0}'.format(opts), cwd=cwd, runas=user) + return _git_run('git rebase {0}'.format(opts), cwd=cwd, runas=user) def checkout(cwd, rev, force=False, opts=None, user=None): ''' @@ -258,7 +270,7 @@ def checkout(cwd, rev, force=False, opts=None, user=None): if not opts: opts = '' cmd = 'git checkout {0} {1} {2}'.format(' -f' if force else '', rev, opts) - return __salt__['cmd.run'](cmd, cwd=cwd, runas=user) + return _git_run(cmd, cwd=cwd, runas=user) def merge(cwd, branch='@{upstream}', opts=None, user=None): ''' @@ -289,7 +301,7 @@ def merge(cwd, branch='@{upstream}', opts=None, user=None): branch, opts) - return __salt__['cmd.run'](cmd, cwd, runas=user) + return _git_run(cmd, cwd, runas=user) def init(cwd, opts=None, user=None): ''' @@ -311,7 +323,7 @@ def init(cwd, opts=None, user=None): _check_git() cmd = 'git init {0} {1}'.format(cwd, opts) - return __salt__['cmd.run'](cmd, runas=user) + return _git_run(cmd, runas=user) def submodule(cwd, init=True, opts=None, user=None): ''' @@ -334,4 +346,4 @@ def submodule(cwd, init=True, opts=None, user=None): if not opts: opts = '' cmd = 'git submodule update {0} {1}'.format('--init' if init else '', opts) - return __salt__['cmd.run'](cmd, cwd=cwd, runas=user) + return _git_run(cmd, cwd=cwd, runas=user) diff --git a/salt/states/git.py b/salt/states/git.py index 87c34855f1..bf63840bf8 100644 --- a/salt/states/git.py +++ b/salt/states/git.py @@ -63,11 +63,12 @@ def latest(name, 'target {0} is found, "git pull" is probably required'.format( target) ) - current_rev = __salt__['git.revision'](target, user=runas) - if not current_rev: + try: + current_rev = __salt__['git.revision'](target, user=runas) + except Exception, exception: return _fail( ret, - 'Seems that {0} is not a valid git repo'.format(target)) + str(exception)) if __opts__['test']: return _neutral_test( ret, @@ -76,13 +77,20 @@ def latest(name, pull_out = __salt__['git.pull'](target, user=runas) - if rev: - __salt__['git.checkout'](target, rev, user=runas) + try: + if rev: + __salt__['git.checkout'](target, rev, user=runas) - if submodules: - __salt__['git.submodule'](target, user=runas, opts='--recursive') + if submodules: + __salt__['git.submodule'](target, user=runas, opts='--recursive') + + new_rev = __salt__['git.revision'](cwd=target, user=runas) + + except Exception, exception: + return _fail( + ret, + str(exception)) - new_rev = __salt__['git.revision'](cwd=target, user=runas) if current_rev != new_rev: log.info('Repository {0} updated: {1} => {2}'.format(target, current_rev, @@ -90,13 +98,6 @@ def latest(name, ret['comment'] = 'Repository {0} updated'.format(target) ret['changes']['revision'] = '{0} => {1}'.format( current_rev, new_rev) - else: - # Check that there was no error preventing the revision update - if 'error:' in pull_out: - return _fail( - ret, - 'An error was thrown by git:\n{0}'.format(pull_out) - ) else: if os.path.isdir(target): # git clone is required, but target exists -- however it is empty @@ -125,18 +126,22 @@ def latest(name, ret, 'Repository {0} is about to be cloned to {1}'.format( name, target)) - # make the clone - result = __salt__['git.clone'](target, name, user=runas) - if not os.path.isdir(target): - return _fail(ret, result) + try: + # make the clone + result = __salt__['git.clone'](target, name, user=runas) - if rev: - __salt__['git.checkout'](target, rev, user=runas) + if rev: + __salt__['git.checkout'](target, rev, user=runas) - if submodules: - __salt__['git.submodule'](target, user=runas) + if submodules: + __salt__['git.submodule'](target, user=runas) - new_rev = __salt__['git.revision'](cwd=target, user=runas) + new_rev = __salt__['git.revision'](cwd=target, user=runas) + + except Exception, exception: + return _fail( + ret, + str(exception)) message = 'Repository {0} cloned to {1}'.format(name, target) log.info(message) From 79cacbad1ef7e10e804d25c5bee7790c60bef02f Mon Sep 17 00:00:00 2001 From: ffa Date: Wed, 31 Oct 2012 22:33:07 -0700 Subject: [PATCH 2/5] bug fix and integration tests for git --- salt/modules/git.py | 4 +-- tests/integration/states/git.py | 48 +++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) create mode 100644 tests/integration/states/git.py diff --git a/salt/modules/git.py b/salt/modules/git.py index ddb6eb1b42..21079245b6 100644 --- a/salt/modules/git.py +++ b/salt/modules/git.py @@ -5,7 +5,7 @@ Support for the Git SCM import os from salt import utils -def _git_run(**kwargs): +def _git_run(cmd, cwd=None, **kwargs): ''' simple, throw an exception with the error message on an error return code. @@ -13,7 +13,7 @@ def _git_run(**kwargs): 'cmd.run_all', and used as an alternative to 'cmd.run_all'. Some commands don't return proper retcodes, so this can't replace 'cmd.run_all'. ''' - result = __salt__['cmd.run_all'](**kwargs) + result = __salt__['cmd.run_all'](cmd, cwd=None, **kwargs) retcode = result['retcode'] diff --git a/tests/integration/states/git.py b/tests/integration/states/git.py new file mode 100644 index 0000000000..8f20556fd4 --- /dev/null +++ b/tests/integration/states/git.py @@ -0,0 +1,48 @@ +''' +Tests for the Git state +''' +import os +import shutil +import integration + + +class GitTest(integration.ModuleCase): + ''' + Validate the git state + ''' + + def test_latest(self): + ''' + git.latest + ''' + name = os.path.join(integration.TMP, 'salt_repo') + ret = self.run_state( + 'git.latest', + name='https://github.com/saltstack/salt.git', + rev='develop', + target=name, + submodules=True + ) + self.assertTrue(os.path.isdir(os.path.join(name, '.git'))) + result = self.state_result(ret) + self.assertTrue(result) + shutil.rmtree(name, ignore_errors=True) + + def test_present(self): + ''' + git.present + ''' + name = os.path.join(integration.TMP, 'salt_repo') + ret = self.run_state( + 'git.present', + name=name, + bare=True + ) + self.assertTrue(os.path.isfile(os.path.join(name, 'HEAD'))) + result = self.state_result(ret) + self.assertTrue(result) + shutil.rmtree(name, ignore_errors=True) + +if __name__ == '__main__': + from integration import run_tests + run_tests(GitTest) From 7ab7c280874826d4239a697146fb67a6a474dd49 Mon Sep 17 00:00:00 2001 From: ffa Date: Wed, 31 Oct 2012 22:36:04 -0700 Subject: [PATCH 3/5] Use a salt extended exception --- salt/modules/git.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/salt/modules/git.py b/salt/modules/git.py index 21079245b6..23f800e140 100644 --- a/salt/modules/git.py +++ b/salt/modules/git.py @@ -20,7 +20,7 @@ def _git_run(cmd, cwd=None, **kwargs): if retcode == 0: return result['stdout'] else: - raise Exception(result['stderr']) + raise CommandExecutionError(result['stderr']) def _git_getdir(cwd, user=None): ''' From 7625ebe42976ab14e6f2053a7291df8adbdb8005 Mon Sep 17 00:00:00 2001 From: ffa Date: Wed, 31 Oct 2012 22:41:43 -0700 Subject: [PATCH 4/5] add integration test to test failure --- tests/integration/states/git.py | 35 +++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/integration/states/git.py b/tests/integration/states/git.py index 8f20556fd4..c3b49e5778 100644 --- a/tests/integration/states/git.py +++ b/tests/integration/states/git.py @@ -28,6 +28,23 @@ class GitTest(integration.ModuleCase): self.assertTrue(result) shutil.rmtree(name, ignore_errors=True) + def test_latest_failure(self): + ''' + git.latest + ''' + name = os.path.join(integration.TMP, 'salt_repo') + ret = self.run_state( + 'git.latest', + name='https://youSpelledGithubWrong.com/saltstack/salt.git', + rev='develop', + target=name, + submodules=True + ) + self.assertFalse(os.path.isdir(os.path.join(name, '.git'))) + result = self.state_result(ret) + self.assertFalse(result) + shutil.rmtree(name, ignore_errors=True) + def test_present(self): ''' git.present @@ -43,6 +60,24 @@ class GitTest(integration.ModuleCase): self.assertTrue(result) shutil.rmtree(name, ignore_errors=True) + def test_present_failure(self): + ''' + git.present + ''' + name = os.path.join(integration.TMP, 'salt_repo') + with file(fname, 'a'): + pass + ret = self.run_state( + 'git.present', + name=name, + bare=True + ) + self.assertFalse(os.path.isfile(os.path.join(name, 'HEAD'))) + result = self.state_result(ret) + self.assertFalse(result) + shutil.rmtree(name, ignore_errors=True) + + if __name__ == '__main__': from integration import run_tests run_tests(GitTest) From 019afc0e80b54752bf1e6eefa77539b12d7d69c5 Mon Sep 17 00:00:00 2001 From: ffa Date: Wed, 31 Oct 2012 23:13:25 -0700 Subject: [PATCH 5/5] misc bug fixes --- salt/modules/git.py | 6 +++--- tests/integration/states/git.py | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/salt/modules/git.py b/salt/modules/git.py index 23f800e140..49f08b005e 100644 --- a/salt/modules/git.py +++ b/salt/modules/git.py @@ -3,7 +3,7 @@ Support for the Git SCM ''' import os -from salt import utils +from salt import utils, exceptions def _git_run(cmd, cwd=None, **kwargs): ''' @@ -13,14 +13,14 @@ def _git_run(cmd, cwd=None, **kwargs): 'cmd.run_all', and used as an alternative to 'cmd.run_all'. Some commands don't return proper retcodes, so this can't replace 'cmd.run_all'. ''' - result = __salt__['cmd.run_all'](cmd, cwd=None, **kwargs) + result = __salt__['cmd.run_all'](cmd, cwd=cwd, **kwargs) retcode = result['retcode'] if retcode == 0: return result['stdout'] else: - raise CommandExecutionError(result['stderr']) + raise exceptions.CommandExecutionError(result['stderr']) def _git_getdir(cwd, user=None): ''' diff --git a/tests/integration/states/git.py b/tests/integration/states/git.py index c3b49e5778..1f5c1170bf 100644 --- a/tests/integration/states/git.py +++ b/tests/integration/states/git.py @@ -65,6 +65,8 @@ class GitTest(integration.ModuleCase): git.present ''' name = os.path.join(integration.TMP, 'salt_repo') + os.mkdir(name) + fname = os.path.join(name, 'stoptheprocess') with file(fname, 'a'): pass ret = self.run_state(