-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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
New module postgresql_query #52555
New module postgresql_query #52555
Conversation
+label postgresql |
The test
|
ready_for_review |
@Dorn- @andytom @b6d @dschep @jbscalia @jensdepuydt @kostiantyn-nemchenko @kustodian @matburt @nerzhul @sebasmannem As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks solid, two minors (no blockers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know much about Postgres, though just one minor comment
import psycopg2 | ||
HAS_PSYCOPG2 = True | ||
except ImportError: | ||
HAS_PSYCOPG2 = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are you checking this?
In Ansible 2.8 we added ansible/community#346 (comment) which hopefully will give give clearer error messages to users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good notice! Thank you, fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gundalow , is it necessary to use traceback here ?
import traceback
PSYCOPG2_IMP_ERR = None
...
PSYCOPG2_IMP_ERR = traceback.format_exc()
I'm asking because I've recently heard that it's not necessary.
If we use module.fail_json(msg=missing_required_lib('psycopg2'), exception=PSYCOPG2_IMP_ERR)
the output is a little different:
The full traceback is:
Traceback (most recent call last):
File "/tmp/ansible_postgresql_ping_payload_HnnFhj/__main__.py", line 113, in <module>
import psycopg2
ImportError: No module named psycopg2
spblnx176 | FAILED! => {
"changed": false,
"invocation": {
"module_args": {
"db": null,
"login_host": "",
"login_password": "",
"login_unix_socket": "",
"login_user": "postgres",
"port": 5432,
"ssl_mode": "prefer",
"ssl_rootcert": null
}
},
"msg": "Failed to import the required Python library (psycopg2) on spblnx176.lan's Python /usr/bin/python. Please read module documentation and install in the appropriate location"
}
module.fail_json(msg=missing_required_lib('psycopg2') without traceback
The full traceback is:
WARNING: The below traceback may *not* be related to the actual failure.
File "/tmp/ansible_postgresql_ping_payload_y3izFs/__main__.py", line 111, in <module>
import psycopg2
spblnx176 | FAILED! => {
"changed": false,
"invocation": {
"module_args": {
"db": null,
"login_host": "",
"login_password": "",
"login_unix_socket": "",
"login_user": "postgres",
"port": 5432,
"ssl_mode": "prefer",
"ssl_rootcert": null
}
},
"msg": "Failed to import the required Python library (psycopg2) on spblnx176.lan's Python /usr/bin/python. Please read module documentation and install in the appropriate location"
}
Should I add traceback to all my new modules including this or not ? (now I use just module.fail_json(msg=missing_required_lib('psycopg2') here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you've got it good, thanks
Hello, the code is looking good to me.
Pros:
I don't have the answer tbh, this is more a philosophy question. Maybe I'm totally wrong, anyway I would be really glad to read your thought. Kind regards. |
The test
|
Hi, @Dorn- . Thank you! There is a lot of things that we can do using shell module but implemented by specific modules. At least, it’s more comfortable. Moreover, imho, it should be implemented for the other database modules too. |
plus we can use returned values |
The test
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shipit
@resmo , thank you for the review! |
SUMMARY
ISSUE TYPE
EXAMPLES
RETURN