Add variables endpoint #141
Conversation
nomad/api/variable.py
Outdated
| if cas: | ||
| params["cas"] = cas |
There was a problem hiding this comment.
This will prevent passing 0 as a cas value. This should probably be cas is not None
There was a problem hiding this comment.
Yep, it's a good point. I found it only when I wrote tests. Thanks!
nomad/api/variable.py
Outdated
| if cas: | ||
| params["cas"] = cas |
There was a problem hiding this comment.
Same feedback as before. This will prevent passing 0 as a cas value. This should probably be cas is not None
| except nomad.api.exceptions.URLNotFoundNomadException: | ||
| return False | ||
|
|
||
| def __getitem__(self, item): |
There was a problem hiding this comment.
Would it not be better to use the get_variable API for this?
It also seems like, though convient, __getitem__ and __contains__ are expensive (making an API call and iterating through all variables) for every check. It also doesn't add value over using the Variable interface.
There is some risk in making it convenient or too easy to do something expensive as it makes it more likely users will do the wrong thing. It might be worth considering either adding a cache or dropping these features in favor of Variable. Or at least using Variable under the hood to avoid the loop here.
There was a problem hiding this comment.
In reading through the rest of the library, it seems like this is a common pattern, so probably best to leave it as is for consistency baring a proposal to shift patterns.
There was a problem hiding this comment.
It was just a draft :)
I changed on get_variables now.
There was a problem hiding this comment.
Hah. Sorry for jumping the gun. I got excited.
There was a problem hiding this comment.
NP :)
Thanks for your help!
Codecov Report
@@ Coverage Diff @@
## master #141 +/- ##
==========================================
+ Coverage 90.47% 91.26% +0.79%
==========================================
Files 28 31 +3
Lines 1155 1271 +116
==========================================
+ Hits 1045 1160 +115
- Misses 110 111 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
@jonathanrcross @jrxFive I finished PR, could you review it, please? When we'll merge the PR I think we can release 1.5.0 (I changed the version in setup.py). @ViViDboarder Could you also re-review, please? It was just a draft and I wasn't ready for review 😄 |
Added support for /var and /vars endpoints:
https://developer.hashicorp.com/nomad/api-docs/variables