Skip to content
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

Offhours holidays #1870

Merged
merged 13 commits into from Dec 5, 2017
Merged

Offhours holidays #1870

merged 13 commits into from Dec 5, 2017

Conversation

hav3wb
Copy link
Contributor

@hav3wb hav3wb commented Dec 4, 2017

No description provided.

@hav3wb
Copy link
Contributor Author

hav3wb commented Dec 4, 2017

Allows for submitting a list of holidays to exclude from either offhours or onhours. Can also specify a url containing the list using the same ValueFrom syntax. I also made sure to include additional tests, a validation check to prevent users from supplying two lists and an updated description provided by the custodian schema ec2.filters.offhour command.

@hav3wb
Copy link
Contributor Author

hav3wb commented Dec 4, 2017

For the sake of consistency, I used dates of the format YYYY-MM-DD to check against as that was the default provided by python datetime. I opted to omit an extra variable to set the syntax of the list as I thought it was unnecessary, but my code could be reformatted to require a variable dictating the syntax of the list (i.e. syntax: "%Y-%m-%d"). Finally, I included year in all of the holidays (must use 2017-12-25 instead of 12-25) because several of the US holidays are celebrated on different days each year. This could cause issues if the list provided is not updated yearly by shutting down instances on the previous year's holiday dates. This way, if it is not updated, then nothing gets accidentally skipped.

Copy link
Collaborator

@kapilt kapilt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!, lgtm, some minors

@@ -376,6 +382,14 @@ def process_resource_schedule(self, i, value, time_type):
return False
now = datetime.datetime.now(tz).replace(
minute=0, second=0, microsecond=0)
now_str = now.date().strftime("%Y-%m-%d")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the date() here is redundant, can just strftime on the datetime

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I just used date to make it clear I was only getting date, but I will switch in my next commit.

@@ -235,6 +236,8 @@ class Time(Filter):
'weekends': {'type': 'boolean'},
'weekends-only': {'type': 'boolean'},
'opt-out': {'type': 'boolean'},
'skip-days': {'type': 'array', 'items': {'type': 'string'}},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you do a regex constraint on the string value for skip-days to ensure the format your expecting.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kapilt Added!

@jantman
Copy link
Contributor

jantman commented Dec 5, 2017

Looks great to me, and is also very timely as we'd been discussing this at my employer lately (as well as how to skip termination on holidays, but that's a much larger scope....).

One request though; do you think you could add some documentation about this to the docstring at the top of c7n/filters/offhours.py? I see that you added some docs to the EC2-specific code, but the large docstring at the top of offhours.py is used (via the sphinx automodule directive) to generate the Example offhours policy page, which is the most complete documentation on the use of on/off hours.

@hav3wb
Copy link
Contributor Author

hav3wb commented Dec 5, 2017

@jantman I updated the offhours documentation and added a separate section for dealing with holidays.

@jantman
Copy link
Contributor

jantman commented Dec 5, 2017

Thanks so much for adding the docs!

Copy link
Collaborator

@kapilt kapilt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks

@kapilt kapilt merged commit d7c44af into cloud-custodian:master Dec 5, 2017
lamyanba pushed a commit to lamyanba/cloud-custodian that referenced this pull request Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants