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 msg if API response can't be converted #486

Merged
merged 7 commits into from
Sep 30, 2024

Conversation

konstruktoid
Copy link

This PR makes the return msg a bit more helpful when the API response can't be converted to json.

Pre- and post-patch:

$ ansible-playbook -ihosts cert.yml 2>/dev/null

PLAY [Clusters_Info playbook] **********************************************************************************************************************

TASK [Getting all clusters - validate_certs True] ***********************************************************************************************************************
fatal: [prism]: FAILED! => {"ansible_facts": {"discovered_interpreter_python": "/usr/bin/python"}, "changed": false, "error": null, "msg": "Failed to convert API response to json", "response": null, "status_code": -1}
...ignoring

TASK [Print clusters] ******************************************************************************************************
ok: [prism] => {
    "msg": {
        "ansible_facts": {
            "discovered_interpreter_python": "/usr/bin/python"
        },
        "changed": false,
        "error": null,
        "failed": true,
        "msg": "Failed to convert API response to json",
        "response": null,
        "status_code": -1
    }
}

TASK [Getting all clusters - validate_certs False] *******************************************************************************************************************
ok: [prism]

TASK [Print clusters] *********************************************************************************************
ok: [prism] => {
    "msg": "API version: 3.1"
}

PLAY RECAP **************************************************************************************************************
prism                      : ok=4    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=1
$ ansible-playbook -ihosts cert.yml 2>/dev/null

PLAY [Clusters_Info playbook] ****************************************************************************************************************************

TASK [Getting all clusters - validate_certs True] ****************************************************************************************************************************
fatal: [prism]: FAILED! => {"ansible_facts": {"discovered_interpreter_python": "/usr/bin/python"}, "changed": false, "error": null, "msg": "Request failed: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:618)>", "response": null, "status_code": -1}
...ignoring

TASK [Print clusters] *****************************************************************************************************************************
ok: [prism] => {
    "msg": {
        "ansible_facts": {
            "discovered_interpreter_python": "/usr/bin/python"
        },
        "changed": false,
        "error": null,
        "failed": true,
        "msg": "Request failed: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:618)>",
        "response": null,
        "status_code": -1
    }
}

TASK [Getting all clusters - validate_certs False] *************************************************************************************************************************
ok: [prism]

TASK [Print clusters] *****************************************************************************************************
ok: [prism] => {
    "msg": "API version: 3.1"
}

PLAY RECAP **************************************************************************************************************
prism                      : ok=4    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=1

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
plugins/module_utils/entity.py Outdated Show resolved Hide resolved
@bhati-pradeep
Copy link
Collaborator

Thank you for raising PR

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
plugins/module_utils/entity.py Outdated Show resolved Hide resolved
plugins/module_utils/entity.py Outdated Show resolved Hide resolved
Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
@konstruktoid
Copy link
Author

Added a try:/catch: and I assume that any error message will be > 2, but it will skip any empty ("") messages.

@marathe-99
Copy link

Is that problem is resolved??

@bhati-pradeep
Copy link
Collaborator

bhati-pradeep commented Aug 2, 2024

Is that problem is resolved??

Not yet @marathe-99, this will go in next maintenance release.

resp_json_msg = "{0}".format(info.get("msg"))
else:
try:
if len(info.get("msg")) > 2:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thats fine, let it be 0 so that we can back trace any error message in our product code base.

Copy link
Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

@bhati-pradeep bhati-pradeep Sep 29, 2024

Choose a reason for hiding this comment

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

@konstruktoid What happens if msg key is not present in info ? I am just thinking it can cause TypeError ? As len(None) can cause that. Does ansible's fetch_url confirms that msg will be always a string ?

Copy link
Collaborator

@bhati-pradeep bhati-pradeep Sep 29, 2024

Choose a reason for hiding this comment

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

Since you are only checking ValueError.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@konstruktoid there is a small syntax issue. Please check latest comment

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
@abhinavbansal29 abhinavbansal29 changed the base branch from main to release/1.9.3 September 23, 2024 06:37
@abhinavbansal29 abhinavbansal29 self-assigned this Sep 23, 2024
Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
bhati-pradeep

This comment was marked as outdated.

plugins/module_utils/entity.py Outdated Show resolved Hide resolved
Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
Copy link
Collaborator

@george-ghawali george-ghawali left a comment

Choose a reason for hiding this comment

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

LGTM

@bhati-pradeep bhati-pradeep changed the base branch from release/1.9.3 to main September 30, 2024 09:34
@bhati-pradeep bhati-pradeep changed the base branch from main to release/1.9.3 September 30, 2024 09:39
Copy link
Collaborator

@bhati-pradeep bhati-pradeep left a comment

Choose a reason for hiding this comment

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

LGTM

@bhati-pradeep bhati-pradeep merged commit 120ba18 into nutanix:release/1.9.3 Sep 30, 2024
1 of 2 checks passed
@konstruktoid konstruktoid deleted the nonejson branch September 30, 2024 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants