-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
c7n_tencentcloud - resources - clb&cbs #7809
c7n_tencentcloud - resources - clb&cbs #7809
Conversation
dict_tag.update({tag["Resource"].split('/')[-1]: tag["Tags"]}) | ||
return dict_tag | ||
|
||
def get_cvm_resource_qcs(self, resources): |
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.
Feels like this should be a method on the resource manager itself, alternatively we could augment resources with a qcs
key at the top level, maybe adding a default augment
method on the QueryResourceManager
.
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.
At the very least, a get_qcs
classmethod seems necessary
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.
A resource sometimes need fetch related resource's metadata, thus we need the qcs.
I think there are some duplicated code for generating qcs, which need to be optimized.
We do it in this PR.
And we plan to have a further refactor soon
example: '2022-09-28T15:28:28+00:00' | ||
""" | ||
for key in field_keys: | ||
dt = timezone_from.localize(datetime.strptime(data[key], original_date_str_format)) |
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.
this feels quite strange, we're localizing, and then converting back to utc? what's the service response tz?
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.
date timezone from tencentcloud is different depending on API, and string format is not always the same, most of them without tz info.
To be simple and safe, we just convert it to utc. Maybe it's not necessary, we can modify it.
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.
if we don't know the origin timezone, we can't convert without ambiguity/correctness concerns. hardcoding it to Shanghai tz isn't right, that could just be an artifact of which region/endpoint is being targeted. we should also try to capture on resource type metadata wrt to which services are returning local tz (either on regional basis) vs utc. it feels like this is a point where it would be good to engage with the service team to have a clearer understanding of the behavior.
…feature/resources_clb_cbs
No description provided.