adde support for v2 api Settings#77
adde support for v2 api Settings#77Dynatrace-James-Kitson merged 5 commits intodynatrace-oss:mainfrom
Conversation
* add unittests * add mock data
|
Wouter De Troyer seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Thank you for your contributions! Overall it looks like a good start on the settings API. There's a few small issues around formatting and consistency which we can clean up. I'll make a few notes, if you'd like to work on those yourself otherwise I can merge and make the changes myself. |
Dynatrace-James-Kitson
left a comment
Dynatrace-James-Kitson
left a comment
There was a problem hiding this comment.
Some formatting using black also should be done (see the contributions guide doc about this).
Overall a very good start and can be merged, but will need some changes before inclusion in a release. I'll give some time if you'd like to make some more changes otherwise I can merge and do on top.
dynatrace/environment_v2/settings.py
Outdated
| from dynatrace.pagination import PaginatedList | ||
| import json | ||
| import logging | ||
| from http.client import HTTPConnection # py3 |
There was a problem hiding this comment.
I don't think this is used anywhere?
dynatrace/environment_v2/settings.py
Outdated
| def __init__(self, http_client: HttpClient): | ||
| self.__http_client = http_client | ||
|
|
||
| def list(self,schema_id: Optional[str] = None, |
There was a problem hiding this comment.
We'll want to update these method names to 'list_objects' etc... The issue would be that this API also lets you list and view 'schemas' so we'd need to name them a bit more clearly to differentiate.
dynatrace/environment_v2/settings.py
Outdated
| } | ||
| return PaginatedList(Settings, self.__http_client, target_url=self.ENDPOINT, list_item="items", target_params=params) | ||
|
|
||
| def post(self,external_id,object_id,schema_id,schema_version,scope, value,validate_only): |
There was a problem hiding this comment.
Similar to above the name should clarify what we're interacting with as well as we want to name them after the action not the HTTP method.
e.g. create_object
dynatrace/environment_v2/settings.py
Outdated
| return self.__http_client.make_request(path=f"{self.ENDPOINT}/{object_id}", method="DELETE") | ||
|
|
||
| class Settings(DynatraceObject): | ||
| value = None |
There was a problem hiding this comment.
I don't think this is needed/used anywhere.
dynatrace/environment_v2/settings.py
Outdated
| # Mandatory | ||
| self.objectId: str = raw_element["objectId"] | ||
| self.value: str = raw_element["value"] | ||
| def to_json(self): |
There was a problem hiding this comment.
I understand the motivation for this but I don't think we'd include this as all it is is a 'getter' that returns the value attribute. If this is needed they can just access the 'value' attribute of the settings object. We don't have anything like this for other objects. to_json() could also confuse someone if they expected the whole json including the object id.
* renamed methods * removed unnecessary methods * updated tests for method rename
|
Kudos, SonarCloud Quality Gate passed!
|
|
I've been adding to this commit this week. Looking through more and testing I've made a few changes to have it align better with the rest of this project and am also deciding on some details about the 'settings object' create behavior to make it line up properly with the API while also being usable. |
3 participants
added v2 settings support and tests