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

overwrite timezone configuration with env #981

Merged
merged 1 commit into from Jul 3, 2019

Conversation

jasonyihk
Copy link
Contributor

@jasonyihk jasonyihk commented Jun 26, 2019

link to issue #982

@coveralls
Copy link

coveralls commented Jun 26, 2019

Pull Request Test Coverage Report for Build 3393

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 73.786%

Totals Coverage Status
Change from base Build 3392: 0.0%
Covered Lines: 3726
Relevant Lines: 4788

💛 - Coveralls

@codecov-io
Copy link

codecov-io commented Jun 26, 2019

Codecov Report

Merging #981 into master will increase coverage by 0.22%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #981      +/-   ##
==========================================
+ Coverage   73.12%   73.34%   +0.22%     
==========================================
  Files         112      112              
  Lines        4788     4764      -24     
  Branches      612      609       -3     
==========================================
- Hits         3501     3494       -7     
+ Misses       1062     1045      -17     
  Partials      225      225
Impacted Files Coverage Δ
tcms/settings/common.py 98.38% <100%> (ø) ⬆️
tcms/telemetry/views.py 100% <0%> (ø) ⬆️
tcms/testruns/models.py 85.47% <0%> (+0.77%) ⬆️
tcms/telemetry/api.py 36.36% <0%> (+8.86%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f85b4ab...f17c777. Read the comment docs.

Copy link
Member

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Overall looks good, minor changes requested.

Also:

  • squash the two commits together and add Fixes #982 in the commit log
  • you may want to update the commit log text to indicate you are adding a new functionality

tcms/settings/common.py Outdated Show resolved Hide resolved
@jasonyihk
Copy link
Contributor Author

general question: when will be the new reporting - telemetry available? this was used quite heavily by our internal QA team and they have to do some manual reporting now after switched to 6.10. any preview branch available?

@atodorov
Copy link
Member

general question: when will be the new reporting - telemetry available?

It will be ready when our resources allow it. Just yesterday we've merged the first iteration of the status matrix and it will become available in the next version that will be released.

this was used quite heavily by our internal QA team and they have to do some manual reporting now after switched to 6.10. any preview branch available?

We've had #657 opened since December and we dropped legacy reports after March 1st. The specs for the new Telemetry have been published since then. I don't see any feedback from you wrt features or information what and how is used so there isn't really anything we can do at this point. Feel free to open a new issue to discuss use-cases and features which are not in the current telemetry spec so we don't pollute this pull request with off-topic comments.

Copy link
Member

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Overall looks good, see the review comment b/c the current piece of code can be improved a bit more.

About commit log message overwrite with env value - overwrite what ?

When somebody sees this commit they will have no idea what it is.

A more descriptive Overwrite timezone settings via env or Use ENV variables to control timezone settings, etc will be better.

@@ -208,11 +208,12 @@
]

# If you set this to False, Django will not use timezone-aware datetimes.
USE_TZ = False
USE_TZ = True if os.environ.get('KIWI_USE_TZ') == str(True) else False
Copy link
Member

Choose a reason for hiding this comment

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

What is your intention here? Do you want to use timezone only when $KIWI_USE_TZ is set to the string "True" or any other non-empty string ?

Depending on what behaviour you want you either need a comment to document that or use a simpler boolean condition (e.g. using == will return bool and you can also compare strings directly, can use .lower() for convenience).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention is to use timezone ONLY when env KIWI_USE_TZ is set to True. non-empty string , e.g. bool('False'), will resolve to true in python. prefer not using .lower() to follow Django settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comment has been updated. re-check.

Copy link
Member

Choose a reason for hiding this comment

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

os.environ.get('KIWI_USE_TZ', 'False').lower() == 'true' evaluates as boolean and allows the user to specify any capitalization variant of "True". If you don't want to allow variations then os.environ.get('KIWI_USE_TZ') == 'True' is still simpler than the if expression you have currently.

Copy link
Contributor Author

@jasonyihk jasonyihk Jul 2, 2019

Choose a reason for hiding this comment

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

if KIWI_USE_TZ is set to False or any non-empty string, os.envuse iron.get('KIWI_USE_TZ') will resolve to True, this is why I use str(True) in the code to make sure only "True" is taken as authentic.

Copy link
Member

Choose a reason for hiding this comment

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

if KIWI_USE_TZ is set to False or any non-empty string, os.envuse iron.get('KIWI_USE_TZ') will resolve to True

This is simply not True. os.environ.get returns a string, it isn't automatically resolved as boolean. This is why you compare the result with == to another string and the result of the equality comparison operator is boolean:

>>> use_tz = os.environ.get('KIWI_USE_TZ')
>>> use_tz
'False'
>>> type(use_tz)
<type 'str'>
>>> use_tz.lower() == 'true'
False

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, updated.

Copy link
Member

@atodorov atodorov Jul 2, 2019

Choose a reason for hiding this comment

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

I don't see an update. This should become:
USE_TZ = os.environ.get('KIWI_USE_TZ', 'False').lower() == 'true'

Copy link
Contributor Author

@jasonyihk jasonyihk Jul 2, 2019

Choose a reason for hiding this comment

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

oh. please re-check.

Copy link
Member

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

See comment about the conditional statement. Everything else looks good.

@@ -208,11 +208,12 @@
]

# If you set this to False, Django will not use timezone-aware datetimes.
USE_TZ = False
USE_TZ = True if os.environ.get('KIWI_USE_TZ') == str(True) else False
Copy link
Member

Choose a reason for hiding this comment

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

os.environ.get('KIWI_USE_TZ', 'False').lower() == 'true' evaluates as boolean and allows the user to specify any capitalization variant of "True". If you don't want to allow variations then os.environ.get('KIWI_USE_TZ') == 'True' is still simpler than the if expression you have currently.

@atodorov atodorov merged commit d06f5b3 into kiwitcms:master Jul 3, 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

4 participants