refactor to support multiple cloud providers #119
Conversation
Codecov Report
@@ Coverage Diff @@
## master #119 +/- ##
==========================================
+ Coverage 99.33% 99.41% +0.08%
==========================================
Files 22 27 +5
Lines 905 1034 +129
Branches 48 53 +5
==========================================
+ Hits 899 1028 +129
Misses 6 6
Continue to review full report at Codecov.
|
infinitewarp
force-pushed
the
105-refactor-models
branch
4 times, most recently
from
e6d900f
to
c8dd30f
Compare
infinitewarp
force-pushed
the
105-refactor-models
branch
from
186f9d3
to
acb7e94
Compare
infinitewarp
changed the title
|
|
||
| for event in events: | ||
| product_key = cloud_helper.get_event_product_identifier(event) | ||
| instance_key = cloud_helper.get_event_instance_identifier(event) |
There was a problem hiding this comment.
Does the Django ORM populate the instance object when pulling the initial events or will this make another DB call for each event?
There was a problem hiding this comment.
Django's ORM is generally lazy/deferred in its execution, and it only executes queries when it thinks it needs to. In this case, the act of calling the iterator should have executed the query to get the events, and by including the select_related function in the chain, it should have pre-fetched the related data all at once.
See also:
https://docs.djangoproject.com/en/dev/ref/models/querysets/#select-related
Returns a QuerySet that will “follow” foreign-key relationships, selecting additional related-object data when it executes its query. This is a performance booster which results in a single more complex query but means later use of foreign-key relationships won’t require database queries.
There was a problem hiding this comment.
|
|
||
| class Meta: | ||
| model = Account | ||
| fields = ('id', 'url', 'account_id', 'account_arn') | ||
| read_only_fields = ('account_id', ) | ||
| fields = ( |
There was a problem hiding this comment.
Does this guarantee the order the fields will be serialized in? If so: I like alphabetical, but having the dates together can be beneficial.
There was a problem hiding this comment.
No, and I actually believe DRF always automatically alphabetizes the fields when outputting as JSON. I think I just listed them here alphabetically for consistency sake.
| """ | ||
| if cloud_provider not in CLOUD_PROVIDERS: | ||
| raise InvalidCloudProviderError( | ||
| _('Unsupported cloud provider "{0}".').format(cloud_provider) |
There was a problem hiding this comment.
are we not doing the f'{cloud_provider}' string format method anymore?
There was a problem hiding this comment.
Yes, I mentioned this in a previous PR. I believe f-strings often don't work for gettext because of the order of execution. Which reminds me I need to file an issue to go fix/clean up all our incorrect f-strings…
Take this for example:
astronaut = 'Dave'
message = _(f"I'm sorry, {astronaut}.")
The value passed into the _ function would have already been substituted by the Python interpreter, meaning the translation function gets "I'm sorry, Dave." which should not be found in the translation mapping.
| class AwsReportHelper(ReportHelper): | ||
| """Report helper for AWS.""" | ||
|
|
||
| def assert_account_exists(self): # noqa: D102, see parent class |
There was a problem hiding this comment.
I don't think there is any harm in inserting an aws specific docstring for these methods, better than littering it with noqa tags.
There was a problem hiding this comment.
Yeah, doing this felt iffy, and I was wondering how you guys would feel about it. On the other hand, copy-pasting large swaths of doc strings doesn't feel any better. I wish there was a better supported way to stay DRY with them. 😕
There was a problem hiding this comment.
Maybe not, but in case we ever use anything to auto generate docs from the methods it would still be nice that there would be something written down.
infinitewarp
force-pushed
the
105-refactor-models
branch
from
acb7e94
to
d97bb64
Compare
#105
refactor many things to make more cloud-agnostic