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

USE_TZ = True. #2263

Closed
wants to merge 5 commits into from
Closed

USE_TZ = True. #2263

wants to merge 5 commits into from

Conversation

fossilet
Copy link
Contributor

@fossilet fossilet commented Mar 3, 2016

We are using the following settings regarding timezone:

TIME_ZONE = 'Europe/Warsaw'
USE_TZ = False

When USE_TZ is False, Django would store timestamps in the timezone of TIME_ZONE, and this is how ralph is storing time. After I realised I should change TIME_ZONE, even if I changed TIME_ZONE to 'Asia/Beijing', the stored time are still in the old timezone (GMT+1). How about it that we change USE_TZ to True, to make sue time is always saved as UTC? This is also recommended by Django.

@mkurek
Copy link
Contributor

mkurek commented Mar 3, 2016

Hi @fossilet. We cannot simply enable USE_TZ right now because it breaks existing code. Take a look at travis unit tests - some of them are broken. Its probably caused by using datetime core module of Python instead of django.utils.timezone (https://docs.djangoproject.com/es/1.9/topics/i18n/timezones/#naive-and-aware-datetime-objects) - will you be able to fix these broken places in our code?

@fossilet
Copy link
Contributor Author

fossilet commented Mar 3, 2016

Oops, let me see.

@fossilet
Copy link
Contributor Author

fossilet commented Mar 8, 2016

The difference of the objects in the failed test is two hours. I cannot find why. @mkurek can you have a look at it?

@ghost
Copy link

ghost commented Mar 8, 2016

@fossilet it looks like openstack importer has issue which blocks this pull request.
I'm gonna fix that and we handle your case, ok?

@fossilet
Copy link
Contributor Author

fossilet commented Mar 8, 2016

@xliiv great.

@ghost
Copy link

ghost commented Mar 8, 2016

The bug which I mentioned earlier is because timezone is skipped here:
https://github.com/allegro/ralph/blob/ng/src/ralph/virtual/management/commands/openstack_sync.py#L309
It turned out also that it'd be easier when you merge my changes (then the opposite)
So here's the changes:
ng...xliiv:open-stack-tz
A few tests are broken in my change but I think you already fixed them.
If not give some feedback.

@fossilet
Copy link
Contributor Author

fossilet commented Mar 9, 2016

@xliiv, there is still a flake error even if I have fixed one.

@ar4s
Copy link
Contributor

ar4s commented Mar 9, 2016

@fossilet
Copy link
Contributor Author

fossilet commented Mar 9, 2016

@ar4s, I saw the error, but it is in fact used by the newly added code by @xliiv. That's why I asked him to have a look. Maybe its an error of flake8 itself.

@mkurek
Copy link
Contributor

mkurek commented Mar 9, 2016

@fossilet no it's not used - he replaced datetime.strptime by parser.parse.

@fossilet
Copy link
Contributor Author

I have fixed it.

@@ -306,8 +306,7 @@ def _add_server(self, openstack_server, server_id, project):

# workaround - created field has auto_now_add attribute
new_server.save()
new_server.created = datetime.strptime(openstack_server['created'],
self.DATETIME_FORMAT)
Copy link
Contributor

Choose a reason for hiding this comment

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

self.DATETIME_FORMAT is not used anymore - could you delete it in line 49?

@fossilet
Copy link
Contributor Author

@mkurek I've added it.

@mkurek
Copy link
Contributor

mkurek commented Mar 11, 2016

@fossilet thanks. we'll pull and test your code if it works well with our current setup.

@mkurek mkurek mentioned this pull request Apr 20, 2016
@mkurek
Copy link
Contributor

mkurek commented Apr 20, 2016

Continued in #2391

@mkurek mkurek closed this Apr 20, 2016
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

4 participants