Updates to mark-for-op support for hours #2323
Conversation
c7n/resources/asg.py
Outdated
|
|
||
| default_template = ( | ||
| 'AutoScaleGroup does not meet org policy: {op}@{action_date}') | ||
|
|
||
| def process(self, asgs): | ||
| self.tz = zoneinfo.gettz( |
There was a problem hiding this comment.
this should move to a validate method, ideally we shouldn't even be in policy execution on a config validation error.
There was a problem hiding this comment.
I'm not sure if I quite follow, but I think I understand what you are saying. I think you are saying that by the time we get to process we should almost certain it's going to work, so there should be a validate method to check before we process. I see some examples elsewhere in the code base.
So I think you are asking that:
if not self.tz:
raise FilterValidationError(
"Invalid timezone specified %s" % self.tz)
be moved into a validate method?
I copied this logic from the previous PR that made the updated in tags.py seen here https://github.com/capitalone/cloud-custodian/blob/3a24d34bf59386f0377e984248e77aa28ccef7c5/c7n/tags.py#L608. But looking at the code again, I'm not sure if this if not self.tz is even needed as we set a value right above and default to utc.
Maybe you are asking for:
self.tz = zoneinfo.gettz(
Time.TZ_ALIASES.get(self.data.get('tz', 'utc')))
to be moved out of the process method and then the if not self.tz moved to a validate method?
Any clarification here would be helpful and I'll apply the same to the code in tags.py.
| }, session_factory=session_factory) | ||
| resources = policy.run() | ||
| self.assertEqual(len(resources), 1) | ||
| self.assertEqual(resources[0]['AutoScalingGroupName'], 'marked') |
There was a problem hiding this comment.
ideally this should also construct an asg client and verify the value of the tag, else we're not testing the behavior/implementation just that it ran without errors.
There was a problem hiding this comment.
I've added a test test_asg_mark_for_op_hours that mirrors what I saw for for the ec2 changes, hopefully this is what you are looking for.
c7n/resources/asg.py
Outdated
| # maintains default value of days being 4 if nothing is provided | ||
| days = 4 | ||
| action_date = (n + timedelta(days=days, hours=hours)) | ||
| return action_date.strftime('%Y/%m/%d %H%M %Z') |
There was a problem hiding this comment.
i think we should only retain hours/minutes if hours is set.
|
Thanks for the pr and getting to the cla, looks good, some minors on the review. |
|
Thanks, is there anything else I need to do to get this merged? |
|
all good, thanks |
0leading to unexpected behavior.