-
Notifications
You must be signed in to change notification settings - Fork 631
Address missing months time unit
#120
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
Conversation
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.
Cant this be init'd to
target_date = utc_now
It is all relative to now right?
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.
No, because date objects and datetime objects are different. I have to reference year and month separately here, and I also want each month to be calculated based on the 1st.
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 had that idea too, though. I tried it that way first, and it didn't work. I learned I had to convert 😄
|
LGTM, minor comments |
test_curator/integration/__init__.py
Outdated
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.
His pythonness, Honza, himself put that there. I thought it was odd, but a really cool shortcut now that I know it can be done.
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.
Ah, I see. It's because timedelta uses kwargs. This converts it into the format desired.
Expanded out, it actually would say something like:
step = timedelta(days=1)
So, **{unit: 1} becomes days=1
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.
yep I saw that a soon as I tried writing timedelta(unit=1) in python and it was like NO. ;P
Thanks much for clarifying.
|
+1 on idea, <3 for tests. Any inline comments I made are minor questions. |
Add tests for monthly data Updated CHANGELOG Removed unnecessary client var from calls to Fixed missing newline Added time_unit arguments Filter out '.marvel-kibana' index so it does not appear in logs Update README.md to show as viable option Added datemap with default datestrings. Switched while loop to be range() Changed while loop to range(). Using default datemap from curator.py Updated CHANGELOG
The new
--timestringoption allows for monthly index patterns, e.g.logstash-2014.07, but because python'stimedeltafunction does not recognize months as a time unit, this functionality was omitted from the 1.2.0 release.This PR addresses this completely, with tests. It also patches a few other nits.