review更改 #91

rockzhu · 2018-03-29T02:40:45Z

list 用facts去实现，create返回对象，其他操作返回操作状态

xiaozhu36 · 2018-03-31T12:40:05Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

+short_description: ansible can manage aliyun access_key through this module 
+version_added: "2.5"
+description:
+    - "This module allows the user to manage access-keys. Includes those commands:'present', 'absent'


Line 32行不需要用双引号引起来（况且只有一个双引号），另外，英文介绍最好可以稍微正式点儿

xiaozhu36 · 2018-03-31T12:45:02Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

+  state:
+    description:
+      -  Create,delete or update the access_key of the user. List all access_keys of a user.
+    choices: [ 'present','list','absent']


state的description要跟它的module的功能相对应，这里应该去掉list的部分；choices也一样
每个参数的定义要准确，如果这个参数是必填的，要注明：required: true，如果有默认值，要注明default：“具体的默认值”

xiaozhu36 · 2018-03-31T12:50:42Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

+    type: dict
+    sample: {
+        "access_key_id": "0wNEpMMlzy7szvai",
+        "access_key_secret": "PupkTg8jdmau1cXxYacgE736PJj4cA",


敏感信息不要暴露在开源代码中，用“abc12345“代替就好

xiaozhu36 · 2018-03-31T12:51:14Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

+'''
+
+RETURN = '''
+changed:


变量changed 不用暴露在RETURN中

xiaozhu36 · 2018-03-31T12:58:05Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

+            choices=['present', 'absent'],required=True),
+        user_name = dict(required=True),
+        access_key_ids = dict(required=False),
+        is_active = dict(required=False),


argument_spec 中的参数的定义要跟DOCUMENTATION中的保持一致，如果是required=True，要进行声明，否则不用；如果有choices，要声明，用于做参数合法性校验，如果有default，要声明，为其指定一个初始化的default value；所有的变量的类型要进行声明，具体可参考：https://github.com/ansible/ansible/pull/36898/files#diff-7e74fd025d257f404dbd79c735e2cdd3 这是一个已经提交到官方的并且被官方review过多次的PR

xiaozhu36 · 2018-03-31T13:06:00Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

+    try:
+        changed=ram.delete_access_key(user_name=module.params.get('user_name'),access_key_id=module.params.get('access_key_id'))
+    except Exception as e:
+        module.fail_json(msg="Failed to delete access_key {0}with error {1}".format(module.params.get('access_key_id'),e))


变量之间应该有一个空格进行分割，不然无法通过CI，所以145行应该是：module.fail_json(msg="Failed to delete access_key {0} with error {1}".format(module.params.get('access_key_id'), e))

xiaozhu36 · 2018-03-31T13:37:48Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

+    ram = ram_connect(module)
+    invocations[module.params.get('state')](module, ram)
+
+if __name__ == '__main__':


create和delete实现的逻辑没什么大的问题，由于这个module本身功能不是很大，逻辑不复杂，所以个人认为直接在main中实现create和delete就可以了，不用单独定义三个函数，可以避免重复多次的取值和判断的逻辑，同时整体代码看起来也很整洁。

xiaozhu36 · 2018-03-31T13:41:02Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key_facts.py

+        {
+          "access_key_id": "0wNEpMMlzy7szvai",
+            "status": "Active",
+            "create_date": "2015-01-23T12:33:18Z"


参数上下要对齐

xiaozhu36 · 2018-03-31T13:41:49Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key_facts.py

+    result = []
+    ids = []
+
+    if user_name:


有了124行的required=True，135行的逻辑可以省略

xiaozhu36 · 2018-03-31T13:42:43Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

+    ram = ram_connect(module)
+    invocations[module.params.get('state')](module, ram)
+
+if __name__ == '__main__':


两个函数之间有且只有两个空行

xiaozhu36 · 2018-04-18T00:39:02Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

 options:
  state:
    description:
-      -  Create,delete or update the access_key of the user. List all access_keys of a user.
-    choices: [ 'present','list','absent']
+      -  Create,delete or update the access_key of the user. 


create 和 delete之间应该要有一个空格

xiaozhu36 · 2018-04-18T00:43:00Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

-      -  Create,delete or update the access_key of the user. List all access_keys of a user.
-    choices: [ 'present','list','absent']
+      -  Create,delete or update the access_key of the user. 
+    choices: [ 'present', 'absent']


state 统一改为optional，然后default设置为present

xiaozhu36 · 2018-04-18T00:48:14Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

-        access_key_ids = dict(required=False),
-        is_active = dict(required=False),
+        access_key_id = dict(required=False),
+        is_active = dict(default=True, required=False),


argument_spec中申明的参数，要跟DOCUMENT中定义的一样，

如果是required=True，要显示申明，否则不用

如果有default值，要显示申明

如果有choices，要显示申明

除此之外，此处要对参数做类型的显示申明

xiaozhu36 · 2018-04-18T00:51:16Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

@@ -142,7 +141,7 @@ def ali_access_key_del(module,ram):
    try:
        changed=ram.delete_access_key(user_name=module.params.get('user_name'),access_key_id=module.params.get('access_key_id'))
    except Exception as e:
-        module.fail_json(msg="Failed to delete access_key {0}with error {1}".format(module.params.get('access_key_id'),e))
+        module.fail_json(msg="Failed to delete access_key {0} with error {1}".format(module.params.get('access_key_id'),e))
    module.exit_json(changed=changed)

 def validate_parameters(required_vars, valid_vars, module):


函数与函数之间有且只有两个空行

xiaozhu36 · 2018-04-18T00:52:17Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

@@ -18,6 +18,8 @@
 # You should have received a copy of the GNU General Public License
 # along with Ansible. If not, see http://www.gnu.org/licenses/.

+from __future__ import absolute_import, division, print_function
+metaclass = type

 ANSIBLE_METADATA = {'metadata_version': '1.1',
                    'status': ['preview'],


在metadata中要加上“'supported_by': 'community'”

xiaozhu36 · 2018-05-03T03:43:15Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

+        alicloud_region: '{{ alicloud_region }}'
+        user_name:'{{ user_name }}'
+        access_key_ids:'{{ access_key_ids }}'
+        state: 'absent'


变量跟变量值之间应该有一个空格，所以90行应该是 user_name: '{{ user_name }}'; 91行应该是：access_key_ids: '{{ access_key_ids }}' 烦请检查其他的地方

xiaozhu36 · 2018-05-03T03:44:35Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

+def ali_access_key_create(module,ram):
+    #user_name
+    required_vars = ['user_name']
+    valid_vars = ['access_key_id','is_active']


两个item之间，应该保留一个空格：valid_vars = ['access_key_id', 'is_active']，烦请检查其他的地方

xiaozhu36 · 2018-05-03T03:46:12Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

+    changed = False
+    try:
+        if params['access_key_id']:
+            changed=ram.update_access_key(user_name=module.params.get('user_name'), access_key_id=params['access_key_id'],is_active=params['is_active'])


125，129 格式问题

xiaozhu36 · 2018-05-03T03:46:48Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

+    validate_parameters(required_vars, valid_vars, module)
+    changed = False
+    try:
+        changed=ram.delete_access_key(user_name=module.params.get('user_name'),access_key_id=module.params.get('access_key_id'))


145格式问题

xiaozhu36 · 2018-05-03T03:48:09Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

+    state = module.params.get('state')
+    for v in required_vars:
+        if not module.params.get(v):
+            module.fail_json(msg="Parameter %s required for %s state" % (v, state))


message和error的统一为：{}, line 154: 改为：module.fail_json(msg="Parameter {0} required for {1} state".format((v, state))

xiaozhu36 · 2018-05-03T03:48:53Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

+        ]),
+        user_name = dict(type='str', required=True),
+        access_key_id = dict(type='str', required=False),
+        is_active = dict(type='bool', default=True, required=False),


required=False 没必要申明，默认的

xiaozhu36 · 2018-05-03T03:49:21Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

+def main():
+    argument_spec = ecs_argument_spec()
+    argument_spec.update(dict(
+        state=dict(default='present', choices=[


请申明type='str'

xiaozhu36 · 2018-05-03T03:50:02Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key_facts.py

+    type: list
+    sample: [
+        {
+          "access_key_id": "0wNEpMMlzy7szvai",


敏感信息记得脱敏

xiaozhu36 · 2018-05-03T03:50:53Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key_facts.py

+            result.append(get_info(accesskey))
+            module.exit_json(changed=False, access_keys=result, total=len(result))
+    except ECSResponseError as e:
+        module.fail_json(msg='Error in describe access_keys: %s' % str(e))


format格式

xiaozhu36 · 2018-05-07T01:31:45Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key_facts.py

+DOCUMENTATION = '''
+---
+module: alicloud_access_key_facts
+version_added: "2.5"


change it to 2.6

xiaozhu36 · 2018-05-07T01:32:19Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

+    try:
+        changed=ram.delete_access_key(user_name=module.params.get('user_name'), access_key_id=module.params.get('access_key_id'))
+    except Exception as e:
+        module.fail_json(msg="Failed to delete access_key {0} with error {1}".format(module.params.get('access_key_id'),e))


格式问题，最后e之前应该有一个空格

xiaozhu36 · 2018-05-07T01:33:19Z

lib/ansible/modules/cloud/alicloud/alicloud_access_key.py

+short_description: ansible can manage aliyun access_key through this module 
+version_added: "2.6"
+description:
+    - This module allows the user to manage access-keys. Includes those commands:'present', 'absent'


格式问题，commands:后应该有一个空格

CLAassistant · 2019-06-28T02:31:43Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

zhuweif seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

review更改

4da8cc4

xiaozhu36 reviewed Mar 31, 2018

View reviewed changes

review更改2

f84a598

xiaozhu36 reviewed Apr 18, 2018

View reviewed changes

review更改3

7f0711a

xiaozhu36 reviewed May 3, 2018

View reviewed changes

review更改4

663eacf

xiaozhu36 reviewed May 7, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

review更改 #91

review更改 #91

rockzhu commented Mar 29, 2018

xiaozhu36 Mar 31, 2018

xiaozhu36 Mar 31, 2018

xiaozhu36 Mar 31, 2018

xiaozhu36 Mar 31, 2018

xiaozhu36 Mar 31, 2018

xiaozhu36 Mar 31, 2018

xiaozhu36 Mar 31, 2018

xiaozhu36 Mar 31, 2018

xiaozhu36 Mar 31, 2018

xiaozhu36 Mar 31, 2018

xiaozhu36 Apr 18, 2018

xiaozhu36 Apr 18, 2018

xiaozhu36 Apr 18, 2018

xiaozhu36 Apr 18, 2018

xiaozhu36 Apr 18, 2018

xiaozhu36 May 3, 2018

xiaozhu36 May 3, 2018

xiaozhu36 May 3, 2018

xiaozhu36 May 3, 2018

xiaozhu36 May 3, 2018

xiaozhu36 May 3, 2018

xiaozhu36 May 3, 2018

xiaozhu36 May 3, 2018

xiaozhu36 May 3, 2018

xiaozhu36 May 7, 2018

xiaozhu36 May 7, 2018

xiaozhu36 May 7, 2018

CLAassistant commented Jun 28, 2019 •

edited

Loading

review更改 #91

Are you sure you want to change the base?

review更改 #91

Conversation

rockzhu commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Jun 28, 2019 • edited Loading

CLAassistant commented Jun 28, 2019 •

edited

Loading