Skip to content

Conversation

@fumblehool
Copy link
Contributor

No description provided.

@jonathanrcross
Copy link
Collaborator

jonathanrcross commented Jun 1, 2021

Hi @fumblehool, Thanks for putting this PR. I was curious if we should start adding **kwargs to each method since the Nomad API is still changing and that having to keep adding and figuring out which parameters are compatible for which version may be an impossible task. So in this case it would look a bit like this

   def get_jobs(self, prefix=None, **kwargs):
        """ Lists all the jobs registered with Nomad.
           https://www.nomadproject.io/docs/http/jobs.html
            arguments:
              - prefix :(str) optional, specifies a string to filter jobs on based on an prefix.
                        This is specified as a querystring parameter.
              **kwargs: (dict), of querystring parameters to be sent.
            returns: list
            raises:
              - nomad.api.exceptions.BaseNomadException
              - nomad.api.exceptions.URLNotFoundNomadException
        """
get_jobs(self, prefix=None, namespace="some-namespace")

Merging dicts in python 2.7 has a couple more steps than python 3.5+, we'll have a helper utility function that we reference, instead of doing the below in each method.

params = {"prefix": ""}
kwargs = {"namespace": "some-namespace"}

merged_params = params.copy()
merged_params.update(kwargs)
merged_params
{'prefix': '', 'namespace': 'some-namespace'}

return self.request(method="get", params=merged_params).json()

@resmo
Copy link
Collaborator

resmo commented Jun 4, 2021

@jonathanrcross I would like to merge it as is. Any objections?

@jonathanrcross
Copy link
Collaborator

Yup, all good to merge! Thanks @fumblehool! I'll follow up with a PR with what was described.

@resmo resmo merged commit 0d462bd into jrxFive:master Jun 6, 2021
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.

3 participants