rndmh3ro commented 3 years ago

fixes #118

jonathanrcross commented 3 years ago

Hi @rndmh3ro, thanks for the PR! I took a look at this and at first glance -- I thought it would work-- but further inspected base.py:

https://github.com/jrxFive/python-nomad/blob/0d462bd05d12a7c9ad310d5b66cd4897e58a41b0/nomad/api/base.py#L64-L73

which is called by base.py._request as a pre-step before issuing the actual http call: https://github.com/jrxFive/python-nomad/blob/0d462bd05d12a7c9ad310d5b66cd4897e58a41b0/nomad/api/base.py#L89-L96

The issue is that if when instantiating a nomad.Nomad object if the namespace parameter was given, this will currently always override whatever you pass as the namespace parameter via method. If you aren't passing namespace parameter this will currently work but obviously we know of an edge case where it doesn't.

We can change _query_string_builder to have a parameter for the params and check if namespace or region are already declared and avoid overriding them.

Additionally we should add a couple tests to get_job to verify we are indeed requesting the namespace we think we are! I can certainly help out with the changes if needed. A current workaround if this is blocking you is to create different nomad.Nomad objects with the namespace as part of the constructor you want to target.

codecov-commenter commented 3 years ago

Codecov Report

Merging #119 (0d462bd) into master (0d462bd) will not change coverage. The diff coverage is n/a.

:exclamation: Current head 0d462bd differs from pull request most recent head 605bf61. Consider uploading reports for the commit 605bf61 to get more accurate results

@@           Coverage Diff           @@
##           master     #119   +/-   ##
=======================================
  Coverage   78.80%   78.80%           
=======================================
  Files          26       26           
  Lines        1203     1203           
=======================================
  Hits          948      948           
  Misses        255      255

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0d462bd...605bf61. Read the comment docs.

jrxFive commented 3 years ago

hi @rndmh3ro, I worked #120 this should allow us to merge your PR! Ideally if you do have time it be great if you could add a test to verify from the get_jobs level --probably using a mock/responses -- to verify that the querystring is indeed what is being passed. If not we can merge and ill add a test in a follow up. Thanks again!

jrxFive commented 3 years ago

Hi @rndmh3ro I added/merged #120 and #121 based on your PR! Thank you for the your contribution.

rndmh3ro commented 2 years ago

Thank you for doing this!

jrxFive / python-nomad

add support for namespace argument in get_job #119

Codecov Report