Fix `nonetype` by adding `ensure_required_libs` to `get_conn_params` in postgres.py #253

jchancojr · 2022-05-02T15:08:09Z

SUMMARY

Fix nonetype by adding ensure_required_libs to get_conn_params in postgres.py

Fixes #252

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

All

… & 2.10

…ty.postgresql

…ty.postgresql * 'main' of https://github.com/ansible-collections/community.postgresql: Release 2.1.4 commit add new docsite links feature (ansible-collections#248)

added `ensure_required_libs(module)` to `get_conn_params` to ensure a meaningful error is thrown when psycopg2 is not installed. Without this change the error was; `AttributeError: 'NoneType' object has no attribute '__version__'`. Fixes ansible-collections#252

Andersson007

@jchancojr thanks for fixing this!

changelogs/fragments/252-fix-none-attribute-error.yml

plugins/module_utils/postgres.py

…rather than just the definition for clarity.

jchancojr · 2022-05-03T23:56:00Z

Hi, @Andersson007.

Went with your suggestion and added comments above the ensure_required_libs(module) before connect to DB. Let me know if there are any issues :)

Thanks!

hunleyd · 2022-05-04T00:21:03Z

hey @Andersson007 does this pr show conflicts for you? it does for me, but not @jchancojr

also, Jim can't see the actual error log output of the tests, so he can't debug any failures :(

Andersson007 · 2022-05-04T07:57:54Z

hey @Andersson007 does this pr show conflicts for you? it does for me, but not @jchancojr

@hunleyd i see no conflicts

also, Jim can't see the actual error log output of the tests, so he can't debug any failures :(

Do you mean he can't click the checks to see details?

Andersson007 · 2022-05-04T07:58:42Z

@jchancojr thanks!
Also feel free to remove ensure_required_libs from connect_to_db not to invoke it twice. After that we can merge the PR

…led explicitly in each module.

Andersson007 · 2022-05-04T15:16:24Z

tests/unit/plugins/module_utils/test_postgres.py::TestConnectToDb

please take a look at this unit test. I think it should be adjusted a little bit (i.e. remove everything related to connect_to_db)
you can run them locally before pushing with ansible-test units /path/to/file --docker

jchancojr · 2022-05-04T20:29:58Z

please take a look at this unit test. I think it should be adjusted a little bit (i.e. remove everything related to connect_to_db)

New to ansible-test and wasn't able to point it directly at the test_postgres.py file for some reason, but I was able to run ansible-test utils --docker -v on the whole of it, and everything passed.

Example:

[gw6] [ 23%] PASSED tests/unit/plugins/module_utils/test_postgres.py::TestGetConnParams::test_get_conn_params_warn_db_def_false

In the test_postgres.py file I removed the entire call to connect_to_db.

Could use some guidance here probably as I'm not convinced I've done this correctly.

Andersson007 · 2022-05-05T05:00:10Z

ah, sorry, i wanted to say remove stuff related to ensure_required_libs from TestConnectToDB:( my bad. I'll raise a PR. It was a tough day yesterday:)

Andersson007 · 2022-05-05T05:38:25Z

#258

Andersson007

LGTM @jchancojr thanks for fixing this!

Andersson007 · 2022-05-05T05:53:10Z

Guidance from me:

when refactoring something and that something is covered by units, they will probably fail
so it makes sense to run ansible-test units --docker (it will run all tests) or ansible-test units /path/to/file/ --docker
if they fail locally, try to fix them:) in our case we removed the function that checked something and failed if the condition didn't satisfy (in this case, version of psycopg2)
the failing check mocked and passed psycopg2 of older version than required, so the check expected the function to fail
as we don't have ensure_required_lib in there, the check's expectation is not valid anymore, so it failed
so we removed that check (btw ensure_required_libs is also covered separately in that test file)
it also makes sense to run ansible-test sanity /path/to/changed/or/added/file --docker locally before pushing

If any questions, just ask

Andersson007 · 2022-05-05T06:05:26Z

pytest + unit testing principles are good things to learn. The collection requirements (to be in the package) say that a collection must be covered by units or integration tests.
Some time ago i started to cover things (new stuff and when refactoring) with both and i've concluded that it's really worth doing because:

it helped me catch a lot of hidden bugs that i didn't catch with integration tests.
it helps make your design better (forces you to write really simple functions that are easy to test).
though it's a recommendation (to cover code with both types), not a requirement.
unfortunately most of the stuff here is written not in a good manner because it was a long time ago... but, on the other hand, we have space to improve things:)
if you wanna try to write something big from scratch for practice, welcome to https://github.com/ansible-collections/community.cockroachdb :)

…ty.postgresql * 'main' of https://github.com/ansible-collections/community.postgresql: Remove changelog fragment for doc change. Update changelog to reflect doc changes for postgresql_owner.py Change documentation to be more clear that the changes take place for the entire cluster/instance rather than just a single DB. Fix `nonetype` by adding `ensure_required_libs` to `get_conn_params` in postgres.py (ansible-collections#253) Update unit tests (ansible-collections#258) docs: announce the new matrix pg room (ansible-collections#254)

jchancojr added 8 commits April 26, 2022 11:02

Update README.md to remove Ansible 2.9 & 2.10

b744493

Update version.py comment to remove references to Ansible version 2.9…

77dcf0d

… & 2.10

Update changelog

d2cff57

Updated to include feedback from @Andersson007i

4c48c9b

Merge branch 'main' of https://github.com/ansible-collections/communi…

bc2a6a4

…ty.postgresql

Merge branch 'main' of https://github.com/ansible-collections/communi…

ae10d39

…ty.postgresql * 'main' of https://github.com/ansible-collections/community.postgresql: Release 2.1.4 commit add new docsite links feature (ansible-collections#248)

add changelog fragment

8274878

jchancojr requested review from Andersson007 and hunleyd as code owners May 2, 2022 15:08

jchancojr self-assigned this May 2, 2022

Andersson007 reviewed May 3, 2022

View reviewed changes

changelogs/fragments/252-fix-none-attribute-error.yml Outdated Show resolved Hide resolved

plugins/module_utils/postgres.py Outdated Show resolved Hide resolved

jchancojr added 2 commits May 3, 2022 19:24

Update changelog fragment based on Andrew's feedback.

b853767

change the ensure_required_libs(module) call to be in every module …

ad06d40

…rather than just the definition for clarity.

jchancojr requested a review from Andersson007 May 3, 2022 23:54

Add ensure_required_libs to import. Was missing from my last commit.

f4d515e

Fix module to be self.module

0629001

Remove redundant ensure_required_libs(module) call since it's now cal…

7c2301d

…led explicitly in each module.

Andersson007 mentioned this pull request May 5, 2022

Update unit tests #258

Merged

Andersson007 closed this May 5, 2022

Andersson007 reopened this May 5, 2022

Andersson007 approved these changes May 5, 2022

View reviewed changes

Andersson007 merged commit a7f68b8 into ansible-collections:main May 5, 2022

Andersson007 mentioned this pull request May 6, 2022

AttributeError: 'NoneType' object has no attribute '__version__' #252

Closed

jchancojr deleted the issue_252 branch May 11, 2022 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `nonetype` by adding `ensure_required_libs` to `get_conn_params` in postgres.py #253

Fix `nonetype` by adding `ensure_required_libs` to `get_conn_params` in postgres.py #253

jchancojr commented May 2, 2022

Andersson007 left a comment

jchancojr commented May 3, 2022

hunleyd commented May 4, 2022

Andersson007 commented May 4, 2022

Andersson007 commented May 4, 2022 •

edited

Loading

Andersson007 commented May 4, 2022

jchancojr commented May 4, 2022

Andersson007 commented May 5, 2022 •

edited

Loading

Andersson007 commented May 5, 2022

Andersson007 left a comment

Andersson007 commented May 5, 2022 •

edited

Loading

Andersson007 commented May 5, 2022 •

edited

Loading

Fix nonetype by adding ensure_required_libs to get_conn_params in postgres.py #253

Fix nonetype by adding ensure_required_libs to get_conn_params in postgres.py #253

Conversation

jchancojr commented May 2, 2022

SUMMARY

ISSUE TYPE

COMPONENT NAME

Andersson007 left a comment

Choose a reason for hiding this comment

jchancojr commented May 3, 2022

hunleyd commented May 4, 2022

Andersson007 commented May 4, 2022

Andersson007 commented May 4, 2022 • edited Loading

Andersson007 commented May 4, 2022

jchancojr commented May 4, 2022

Andersson007 commented May 5, 2022 • edited Loading

Andersson007 commented May 5, 2022

Andersson007 left a comment

Choose a reason for hiding this comment

Andersson007 commented May 5, 2022 • edited Loading

Andersson007 commented May 5, 2022 • edited Loading

Fix `nonetype` by adding `ensure_required_libs` to `get_conn_params` in postgres.py #253

Fix `nonetype` by adding `ensure_required_libs` to `get_conn_params` in postgres.py #253

Andersson007 commented May 4, 2022 •

edited

Loading

Andersson007 commented May 5, 2022 •

edited

Loading

Andersson007 commented May 5, 2022 •

edited

Loading

Andersson007 commented May 5, 2022 •

edited

Loading