-
Notifications
You must be signed in to change notification settings - Fork 12
Usage #24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Usage #24
Conversation
# Conflicts: # connect/resource/base.py # connect/resource/fulfillment_automation.py
ExampleTierConfigRequestProcessor renamed to ExampleTierConfigProcessor on example and README.
Codecov Report
@@ Coverage Diff @@
## master #24 +/- ##
==========================================
+ Coverage 92.95% 94.09% +1.14%
==========================================
Files 26 28 +2
Lines 809 1084 +275
==========================================
+ Hits 752 1020 +268
- Misses 57 64 +7
Continue to review full report at Codecov.
|
| return self._load_schema(response, many=False) | ||
|
|
||
| @staticmethod | ||
| def create_spreadsheet(usage_records): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, better to create spreadsheet on base of template downloaded with API instead of hardcoding structure right here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently there is way to download template, but limitations it implies are quite big. First one is that will limit PaaS platform configurations, requieres volume with R/W and the way connectors works, they don't need it at all (is library limitation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ok like this then?
connect/resource/usage_automation.py
Outdated
| def _get_usage_template_download_location(self, product_id): | ||
| # type: (str) -> str | ||
| try: | ||
| response = self.api.get(self.urljoin(self.config.api_url, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it worth to use path= instead of url= for api get,
all these urljoin's looks not really good
should be like:
response = self.api.get(path= '/usage/products/')
response = self.api.get(path=f '/usage/products/{product_id}')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the path argument officially supported by requests? Could not find it in the documentation. I can add it to our API client though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably - good moment to invent that, for both requests and usages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| location = self._get_usage_template_download_location(product.id) | ||
| contents = self._retrieve_usage_template(location) if location else None | ||
| if not location or contents is None: | ||
| msg = 'Error obtaining template usage file from `{}`'.format(location) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
truly speaking failure to find template location and no failure to download template itself - different errors.
and error like
"Error obtaining template usage file from None"
will looks like strange ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. Will split this in two different errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| book = self.create_spreadsheet(usage_records) | ||
| self.upload_spreadsheet(usage_file, book) | ||
|
|
||
| def get_usage_template(self, product): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning string looks strange ... binary blob in a string ...
probably operate with file object?
How to use it - need example, like:
with open("template.xlsx", "w") as ssf:
ssf.write(UsageAutomation().get_usage_template('PRD-xxx-yyy-zzz'))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked this and you are right. A "bytes" object should be returned.
It works like this:
- The API '/usage/products/{product_id}/template/' is used to retrieve the link to the usage template (I just spotted a bug in the code here, the '/template/' part is missing; will be fixed in next commit).
- We try to read the contents of the location provided. The contents of the file are returned as text, but since it will be an xslx file, it makes more sense to use the "content" field of the response rather than the "text" field, and obtain it as a bytes object.
Apart from using requests to obtain the file contents, a simple file read is also attempted, in case that the file is stored locally. I think that this should actually only happen during testing, and since there are other alternatives to achieve this during testing (by mocking the requests.get function to return the contents of a file), I didn't come up with the best solution here. I will remove the part where the file is loaded and use requests only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done,
… Before, the URL sent contained the path '/usage/files/usage/products/' instead of '/usage/files/'.
Support for Usage API.