Fixing resources
diff --git a/_modules/gerrit.py b/_modules/gerrit.py
index 2f520b9..db3cdcb 100644
--- a/_modules/gerrit.py
+++ b/_modules/gerrit.py
@@ -62,34 +62,10 @@
 __opts__ = {}
 
 
-# COMMON
+# Common functions
 
 
-def value_from_param(field, spec, param_value):
-    if 'choices' in spec:
-        if param_value not in spec['choices']:
-            raise ValueError(
-                "'%s' is not valid for field %s" % (param_value, field))
-        value = param_value.upper()
-    else:
-        value = param_value
-    return value
-
-
-def value_from_config_info(field, spec, info_value):
-    if isinstance(info_value, dict):
-        # This is a ConfigParameterInfo field. We need to figure out if the
-        # value is TRUE, FALSE or INHERIT.
-        if 'configured_value' in info_value:
-            value = info_value['configured_value']
-        else:
-            value = 'INHERIT'
-    else:
-        value = info_value
-    return value
-
-
-def get_boolean(gerrit, path):
+def _get_boolean(gerrit, path):
     response = gerrit.get(path)
     if response == 'ok':
         value = True
@@ -101,12 +77,12 @@
     return value
 
 
-def get_list(gerrit, path):
+def _get_list(gerrit, path):
     values = gerrit.get(path)
     return values
 
 
-def get_string(gerrit, path):
+def _get_string(gerrit, path):
     try:
         value = gerrit.get(path)
     except requests.exceptions.HTTPError as e:
@@ -119,14 +95,14 @@
     return value
 
 
-def set_boolean(gerrit, path, value):
+def _set_boolean(gerrit, path, value):
     if value:
         gerrit.put(path)
     else:
         gerrit.delete(path)
 
 
-def set_string(gerrit, path, value, field_name=None):
+def _set_string(gerrit, path, value, field_name=None):
     field_name = field_name or os.path.basename(path)
 
     # Setting to '' is equivalent to deleting, so we have no need for the
@@ -136,42 +112,304 @@
     gerrit.put(path, data=data, headers=headers)
 
 
-def maybe_update_field(gerrit, path, field, gerrit_value, ansible_value,
+def _maybe_update_field(gerrit, path, field, gerrit_value, salt_value,
                        type='str', gerrit_api_path=None):
 
     gerrit_api_path = gerrit_api_path or field
     fullpath = path + '/' + gerrit_api_path
 
-    if gerrit_value == ansible_value:
+    if gerrit_value == salt_value:
         logging.info("Not updating %s: same value specified: %s", fullpath,
                      gerrit_value)
         value = gerrit_value
         changed = False
-    elif ansible_value is None:
+    elif salt_value is None:
         logging.info("Not updating %s: no value specified, value stays as %s",
                      fullpath, gerrit_value)
         value = gerrit_value
         changed = False
     else:
         logging.info("Changing %s from %s to %s", fullpath, gerrit_value,
-                     ansible_value)
+                     salt_value)
         if type == 'str':
-            set_string(gerrit, fullpath, ansible_value, field_name=field)
+            _set_string(gerrit, fullpath, salt_value, field_name=field)
         elif type == 'bool':
-            set_boolean(gerrit, fullpath, ansible_value)
+            _set_boolean(gerrit, fullpath, salt_value)
         else:
             raise AssertionError("Unknown Ansible parameter type '%s'" % type)
 
-        value = ansible_value
+        value = salt_value
         changed = True
     return value, changed
 
 
-def quote(name):
+def _quote(name):
     return urllib.quote(name, safe="")
 
 
-# END COMMON
+def _account_name2id(gerrit, name=None):
+    # Although we could pass an AccountInput entry here to set details in one
+    # go, it's left up to the _update_group() function, to avoid having a
+    # totally separate code path for create vs. update.
+    info = gerrit.get('/accounts/%s' % _quote(name))
+    return info['_account_id']
+
+
+def _group_name2id(gerrit, name=None):
+    # Although we could pass an AccountInput entry here to set details in one
+    # go, it's left up to the _update_group() function, to avoid having a
+    # totally separate code path for create vs. update.
+    info = gerrit.get('/groups/%s' % _quote(name))
+    return info['id']
+
+
+def _create_group(gerrit, name=None):
+    # Although we could pass an AccountInput entry here to set details in one
+    # go, it's left up to the _update_group() function, to avoid having a
+    # totally separate code path for create vs. update.
+    group_info = gerrit.put('/groups/%s' % _quote(name))
+    return group_info
+
+
+def _create_account(gerrit, username=None):
+    # Although we could pass an AccountInput entry here to set details in one
+    # go, it's left up to the _update_account() function, to avoid having a
+    # totally separate code path for create vs. update.
+    account_info = gerrit.put('/accounts/%s' % _quote(username))
+    return account_info
+
+
+def _create_account_email(gerrit, account_id, email, preferred=False,
+                         no_confirmation=False):
+    logging.info('Creating email %s for account %s', email, account_id)
+
+    email_input = {
+        # Setting 'email' is optional (it's already in the URL) but it's good
+        # to double check that the email is encoded in the URL properly.
+        'email': email,
+        'preferred': preferred,
+        'no_confirmation': no_confirmation,
+    }
+    logging.debug(email_input)
+
+    path = 'accounts/%s/emails/%s' % (account_id, _quote(email))
+    headers = {'content-type': 'application/json'}
+    gerrit.post(path, data=json.dumps(email_input), headers=headers)
+
+
+def _create_account_ssh_key(gerrit, account_id, ssh_public_key):
+    logging.info('Creating SSH key %s for account %s', ssh_public_key,
+                 account_id)
+
+    import requests
+    from pygerrit import decode_response
+
+    path = 'accounts/%s/sshkeys' % (account_id)
+    # gerrit.post(path, data=ssh_public_key)
+
+    kwargs = {
+        "data": ssh_public_key
+    }
+    kwargs.update(gerrit.kwargs.copy())
+
+    response = requests.put(gerrit.make_url(path), **kwargs)
+
+    return gerrit.decode_response(response)
+
+
+def _create_group_membership(gerrit, account_id, group_id):
+    logging.info('Creating membership of %s in group %s', account_id, group_id)
+#    group_id = _group_name2id(gerrit, group_id)
+    print group_id
+    import json
+    path = 'groups/%s/members/%s' % (_quote(group_id), account_id)
+    gerrit.put(path, data=json.dumps({}))
+
+
+def _ensure_only_member_of_these_groups(gerrit, account_id, salt_groups):
+    path = 'accounts/%s' % account_id
+    group_info_list = _get_list(gerrit, path + '/groups')
+
+    changed = False
+    gerrit_groups = []
+    for group_info in group_info_list:
+        if group_info['name'] in salt_groups:
+            logging.info("Preserving %s membership of group %s", path,
+                         group_info)
+            gerrit_groups.append(group_info['name'])
+        else:
+            logging.info("Removing %s from group %s", path, group_info)
+            membership_path = 'groups/%s/members/%s' % (
+                _quote(group_info['id']), account_id)
+            try:
+                gerrit.delete(membership_path)
+                changed = True
+            except requests.exceptions.HTTPError as e:
+                if e.response.status_code == 404:
+                    # This is a kludge, it'd be better to work out in advance
+                    # which groups the user is a member of only via membership
+                    # in a different. That's not trivial though with the
+                    # current API Gerrit provides.
+                    logging.info(
+                        "Ignored %s; assuming membership of this group is due "
+                        "to membership of a group that includes it.", e)
+                else:
+                    raise
+
+    # If the user gave group IDs instead of group names, this will
+    # needlessly recreate the membership. The only actual issue will be that
+    # Ansible reports 'changed' when nothing really did change, I think.
+    #
+    # We might receive [""] when the user tries to pass in an empty list, so
+    # handle that.
+    for new_group in set(salt_groups).difference(gerrit_groups):
+        if len(new_group) > 0:
+            _create_group_membership(gerrit, account_id, new_group)
+            gerrit_groups.append(new_group)
+            changed = True
+
+    return gerrit_groups, changed
+
+
+def _ensure_only_one_account_email(gerrit, account_id, email):
+    path = 'accounts/%s' % account_id
+    email_info_list = _get_list(gerrit, path + '/emails')
+
+    changed = False
+    found_email = False
+    for email_info in email_info_list:
+        existing_email = email_info['email']
+        if existing_email == email:
+            # Since we're deleting all emails except this one, there's no need
+            # to care whether it's the 'preferred' one. It soon will be!
+            logging.info("Keeping %s email %s", path, email)
+            found_email = True
+        else:
+            logging.info("Removing %s email %s", path, existing_email)
+            gerrit.delete(path + '/emails/%s' % _quote(existing_email))
+            changed = True
+
+    if len(email) > 0 and not found_email:
+        _create_account_email(gerrit, account_id, email,
+                             preferred=True, no_confirmation=True)
+        changed = True
+
+    return email, changed
+
+
+def _ensure_only_one_account_ssh_key(gerrit, account_id, ssh_public_key):
+    path = 'accounts/%s' % account_id
+    ssh_key_info_list = _get_list(gerrit, path + '/sshkeys')
+
+    changed = False
+    found_ssh_key = False
+    for ssh_key_info in ssh_key_info_list:
+        if ssh_key_info['ssh_public_key'] == ssh_public_key:
+            logging.info("Keeping %s SSH key %s", path, ssh_key_info)
+            found_ssh_key = True
+        else:
+            logging.info("Removing %s SSH key %s", path, ssh_key_info)
+            gerrit.delete(path + '/sshkeys/%i' % ssh_key_info['seq'])
+            changed = True
+
+    if len(ssh_public_key) > 0 and not found_ssh_key:
+        _create_account_ssh_key(gerrit, account_id, ssh_public_key)
+        changed = True
+
+    return ssh_public_key, changed
+
+
+def _update_account(gerrit, username=None, **params):
+    change = False
+
+    try:
+        account_info = gerrit.get('/accounts/%s' % _quote(username))
+    except requests.exceptions.HTTPError as e:
+        if e.response.status_code == 404:
+            logging.info("Account %s not found, creating it.", username)
+            account_info = _create_account(gerrit, username)
+            change = True
+        else:
+            raise
+
+    logging.debug(
+        'Existing account info for account %s: %s', username,
+        json.dumps(account_info, indent=4))
+
+    account_id = account_info['_account_id']
+    path = 'accounts/%s' % account_id
+
+    output = {}
+    output['username'] = username
+    output['id'] = account_id
+
+    fullname, fullname_changed = _maybe_update_field(
+        gerrit, path, 'name', account_info.get('name'), params.get('fullname'))
+    output['fullname'] = fullname
+    change |= fullname_changed
+
+    # Set the value of params that the user did not provide to None.
+
+    if params.get('active') is not None:
+        active = _get_boolean(gerrit, path + '/active')
+        active, active_changed = _maybe_update_field(
+            gerrit, path, 'active', active, params['active'], type='bool')
+        output['active'] = active
+        change |= active_changed
+
+    if params.get('email') is not None:
+        email, emails_changed = _ensure_only_one_account_email(
+            gerrit, account_id, params['email'])
+        output['email'] = email
+        change |= emails_changed
+
+    if params.get('groups') is not None:
+        groups, groups_changed = _ensure_only_member_of_these_groups(
+            gerrit, account_info.get('name'), params['groups'])
+        output['groups'] = groups
+        change |= groups_changed
+
+    if params.get('http_password') is not None:
+        http_password = _get_string(gerrit, path + '/password.http')
+        http_password, http_password_changed = _maybe_update_field(
+            gerrit, path, 'http_password', http_password,
+            params.get('http_password'),
+            gerrit_api_path='password.http')
+        output['http_password'] = http_password
+        change |= http_password_changed
+
+    if params.get('ssh_key') is not None:
+        ssh_key, ssh_keys_changed = _ensure_only_one_account_ssh_key(
+            gerrit, account_id,  params['ssh_key'])
+        output['ssh_key'] = ssh_key
+        change |= ssh_keys_changed
+
+    return output, change
+
+
+def _update_group(gerrit, name=None, **params):
+    change = False
+
+    try:
+        group_info = gerrit.get('/groups/%s' % _quote(name))
+    except requests.exceptions.HTTPError as e:
+        if e.response.status_code == 404:
+            logging.info("Group %s not found, creating it.", name)
+            group_info = _create_group(gerrit, name)
+            change = True
+        else:
+            raise
+
+    logging.debug(
+        'Existing info for group %s: %s', name,
+        json.dumps(group_info, indent=4))
+
+    output = {group_info['name']: group_info}
+
+    return output, change
+
+
+# Gerrit client connectors
 
 
 def _gerrit_ssh_connection(**connection_args):
@@ -224,253 +462,7 @@
     return gerrit
 
 
-def _account_name2id(gerrit, name=None):
-    # Although we could pass an AccountInput entry here to set details in one
-    # go, it's left up to the _update_group() function, to avoid having a
-    # totally separate code path for create vs. update.
-    info = gerrit.get('/accounts/%s' % quote(name))
-    return info['_account_id']
-
-
-def _group_name2id(gerrit, name=None):
-    # Although we could pass an AccountInput entry here to set details in one
-    # go, it's left up to the _update_group() function, to avoid having a
-    # totally separate code path for create vs. update.
-    info = gerrit.get('/groups/%s' % quote(name))
-    return info['id']
-
-
-def _create_group(gerrit, name=None):
-    # Although we could pass an AccountInput entry here to set details in one
-    # go, it's left up to the _update_group() function, to avoid having a
-    # totally separate code path for create vs. update.
-    group_info = gerrit.put('/groups/%s' % quote(name))
-    return group_info
-
-
-def create_account(gerrit, username=None):
-    # Although we could pass an AccountInput entry here to set details in one
-    # go, it's left up to the _update_account() function, to avoid having a
-    # totally separate code path for create vs. update.
-    account_info = gerrit.put('/accounts/%s' % quote(username))
-    return account_info
-
-
-def create_account_email(gerrit, account_id, email, preferred=False,
-                         no_confirmation=False):
-    logging.info('Creating email %s for account %s', email, account_id)
-
-    email_input = {
-        # Setting 'email' is optional (it's already in the URL) but it's good
-        # to double check that the email is encoded in the URL properly.
-        'email': email,
-        'preferred': preferred,
-        'no_confirmation': no_confirmation,
-    }
-    logging.debug(email_input)
-
-    path = 'accounts/%s/emails/%s' % (account_id, quote(email))
-    headers = {'content-type': 'application/json'}
-    gerrit.post(path, data=json.dumps(email_input), headers=headers)
-
-
-def create_account_ssh_key(gerrit, account_id, ssh_public_key):
-    logging.info('Creating SSH key %s for account %s', ssh_public_key,
-                 account_id)
-
-    path = 'accounts/%s/sshkeys' % (account_id)
-    gerrit.post(path, data=ssh_public_key)
-
-
-def create_group_membership(gerrit, account_id, group_id):
-    logging.info('Creating membership of %s in group %s', account_id, group_id)
-#    group_id = _group_name2id(gerrit, group_id)
-    print group_id
-    path = 'groups/%s/members/%s' % (quote(group_id), account_id)
-    gerrit.put(path)
-
-
-def ensure_only_member_of_these_groups(gerrit, account_id, ansible_groups):
-    path = 'accounts/%s' % account_id
-    group_info_list = get_list(gerrit, path + '/groups')
-
-    changed = False
-    gerrit_groups = []
-    for group_info in group_info_list:
-        if group_info['name'] in ansible_groups:
-            logging.info("Preserving %s membership of group %s", path,
-                         group_info)
-            gerrit_groups.append(group_info['name'])
-        else:
-            logging.info("Removing %s from group %s", path, group_info)
-            membership_path = 'groups/%s/members/%s' % (
-                quote(group_info['id']), account_id)
-            try:
-                gerrit.delete(membership_path)
-                changed = True
-            except requests.exceptions.HTTPError as e:
-                if e.response.status_code == 404:
-                    # This is a kludge, it'd be better to work out in advance
-                    # which groups the user is a member of only via membership
-                    # in a different. That's not trivial though with the
-                    # current API Gerrit provides.
-                    logging.info(
-                        "Ignored %s; assuming membership of this group is due "
-                        "to membership of a group that includes it.", e)
-                else:
-                    raise
-
-    # If the user gave group IDs instead of group names, this will
-    # needlessly recreate the membership. The only actual issue will be that
-    # Ansible reports 'changed' when nothing really did change, I think.
-    #
-    # We might receive [""] when the user tries to pass in an empty list, so
-    # handle that.
-    for new_group in set(ansible_groups).difference(gerrit_groups):
-        if len(new_group) > 0:
-            create_group_membership(gerrit, account_id, new_group)
-            gerrit_groups.append(new_group)
-            changed = True
-
-    return gerrit_groups, changed
-
-
-def ensure_only_one_account_email(gerrit, account_id, email):
-    path = 'accounts/%s' % account_id
-    email_info_list = get_list(gerrit, path + '/emails')
-
-    changed = False
-    found_email = False
-    for email_info in email_info_list:
-        existing_email = email_info['email']
-        if existing_email == email:
-            # Since we're deleting all emails except this one, there's no need
-            # to care whether it's the 'preferred' one. It soon will be!
-            logging.info("Keeping %s email %s", path, email)
-            found_email = True
-        else:
-            logging.info("Removing %s email %s", path, existing_email)
-            gerrit.delete(path + '/emails/%s' % quote(existing_email))
-            changed = True
-
-    if len(email) > 0 and not found_email:
-        create_account_email(gerrit, account_id, email,
-                             preferred=True, no_confirmation=True)
-        changed = True
-
-    return email, changed
-
-
-def ensure_only_one_account_ssh_key(gerrit, account_id, ssh_public_key):
-    path = 'accounts/%s' % account_id
-    ssh_key_info_list = get_list(gerrit, path + '/sshkeys')
-
-    changed = False
-    found_ssh_key = False
-    for ssh_key_info in ssh_key_info_list:
-        if ssh_key_info['ssh_public_key'] == ssh_public_key:
-            logging.info("Keeping %s SSH key %s", path, ssh_key_info)
-            found_ssh_key = True
-        else:
-            logging.info("Removing %s SSH key %s", path, ssh_key_info)
-            gerrit.delete(path + '/sshkeys/%i' % ssh_key_info['seq'])
-            changed = True
-
-    if len(ssh_public_key) > 0 and not found_ssh_key:
-        create_account_ssh_key(gerrit, account_id, ssh_public_key)
-        changed = True
-
-    return ssh_public_key, changed
-
-
-def _update_account(gerrit, username=None, **params):
-    change = False
-
-    try:
-        account_info = gerrit.get('/accounts/%s' % quote(username))
-    except requests.exceptions.HTTPError as e:
-        if e.response.status_code == 404:
-            logging.info("Account %s not found, creating it.", username)
-            account_info = create_account(gerrit, username)
-            change = True
-        else:
-            raise
-
-    logging.debug(
-        'Existing account info for account %s: %s', username,
-        json.dumps(account_info, indent=4))
-
-    account_id = account_info['_account_id']
-    path = 'accounts/%s' % account_id
-
-    output = {}
-    output['username'] = username
-    output['id'] = account_id
-
-    fullname, fullname_changed = maybe_update_field(
-        gerrit, path, 'name', account_info.get('name'), params.get('fullname'))
-    output['fullname'] = fullname
-    change |= fullname_changed
-
-    # Set the value of params that the user did not provide to None.
-
-    if params.get('active') is not None:
-        active = get_boolean(gerrit, path + '/active')
-        active, active_changed = maybe_update_field(
-            gerrit, path, 'active', active, params['active'], type='bool')
-        output['active'] = active
-        change |= active_changed
-
-    if params.get('email') is not None:
-        email, emails_changed = ensure_only_one_account_email(
-            gerrit, account_id, params['email'])
-        output['email'] = email
-        change |= emails_changed
-
-    if params.get('groups') is not None:
-        groups, groups_changed = ensure_only_member_of_these_groups(
-            gerrit, account_info.get('name'), params['groups'])
-        output['groups'] = groups
-        change |= groups_changed
-
-    if params.get('http_password') is not None:
-        http_password = get_string(gerrit, path + '/password.http')
-        http_password, http_password_changed = maybe_update_field(
-            gerrit, path, 'http_password', http_password,
-            params.get('http_password'),
-            gerrit_api_path='password.http')
-        output['http_password'] = http_password
-        change |= http_password_changed
-
-    if params.get('ssh_key') is not None:
-        ssh_key, ssh_keys_changed = ensure_only_one_account_ssh_key(
-            gerrit, account_id,  params['ssh_key'])
-        output['ssh_key'] = ssh_key
-        change |= ssh_keys_changed
-
-    return output, change
-
-
-def _update_group(gerrit, name=None, **params):
-    change = False
-
-    try:
-        group_info = gerrit.get('/groups/%s' % quote(name))
-    except requests.exceptions.HTTPError as e:
-        if e.response.status_code == 404:
-            logging.info("Group %s not found, creating it.", name)
-            group_info = _create_group(gerrit, name)
-            change = True
-        else:
-            raise
-
-    logging.debug(
-        'Existing info for group %s: %s', name,
-        json.dumps(group_info, indent=4))
-
-    output = {group_info['name']: group_info}
-
-    return output, change
+# Salt modules
 
 
 def account_create(username, fullname=None, email=None, active=None, groups=[], ssh_key=None, http_password=None, **kwargs):
@@ -501,6 +493,7 @@
             'username': username,
             'fullname': fullname,
             'email': email,
+#            'active': active,
             'groups': groups,
             'ssh_key': ssh_key,
             'http_password': http_password
@@ -515,6 +508,7 @@
     :param username: username
     :param fullname: fullname
     :param email: email
+    :param active: active
     :param groups: array of strings
         groups:
             - Non-Interactive Users
@@ -535,6 +529,7 @@
             'username': username,
             'fullname': fullname,
             'email': email,
+#            'active': active,
             'groups': groups,
             'ssh_key': ssh_key,
             'http_password': http_password
@@ -625,7 +620,7 @@
 
     .. code-block:: bash
 
-        salt '*' gerrit.group_create group-name fsdfgwegfe
+        salt '*' gerrit.group_create group-name description
 
     '''
     gerrit_client = _gerrit_http_connection(**kwargs)
diff --git a/_states/gerrit.py b/_states/gerrit.py
index c869d6e..ebd9c6d 100644
--- a/_states/gerrit.py
+++ b/_states/gerrit.py
@@ -47,8 +47,10 @@
     account = __salt__['gerrit.account_get'](name, **kwargs)
 
     if 'Error' not in account:
-        #update account
-        pass
+        # Update account
+        __salt__['gerrit.account_update'](name, fullname, email, active, groups, ssh_key, http_password, **kwargs)
+        ret['comment'] = 'Account "{0}" has been updated'.format(name)
+        ret['changes']['Account'] = 'Updated'
     else:
         # Create account
         __salt__['gerrit.account_create'](name, fullname, email, active, groups, ssh_key, http_password, **kwargs)
@@ -72,7 +74,7 @@
     group = __salt__['gerrit.group_get'](name, **kwargs)
 
     if 'Error' not in group:
-        #update group
+        # Update group
         pass
     else:
         # Create group
diff --git a/gerrit/client/group.sls b/gerrit/client/group.sls
new file mode 100644
index 0000000..e510e1a
--- /dev/null
+++ b/gerrit/client/group.sls
@@ -0,0 +1,15 @@
+{% from "gerrit/map.jinja" import client with context %}
+{%- if client.enabled %}
+
+{%- for group_name, group in client.group.iteritems() %}
+
+gerrit_client_group_{{ group_name }}:
+  gerrit.group_present:
+  - name: {{ group_name }}
+  {%- if group.description is defined %}
+  - description: {{ group.description }}
+  {%- endif %}
+
+{%- endfor %}
+
+{%- endif %}
\ No newline at end of file
diff --git a/gerrit/client/init.sls b/gerrit/client/init.sls
index a05f9df..2c9047c 100644
--- a/gerrit/client/init.sls
+++ b/gerrit/client/init.sls
@@ -1,3 +1,5 @@
 include:
 - gerrit.client.service
+- gerrit.client.group
 - gerrit.client.user
+- gerrit.client.project