fabric8-services / fabric8-auth

Identity and Access Management for fabric8 services
https://auth.openshift.io/api/status
Apache License 2.0
14 stars 26 forks source link

Fix usage of user inactivity configuration functions #839

Closed sbryzak closed 5 years ago

sbryzak commented 5 years ago

Renamed these configuration functions:

GetUserDeactivationInactivityPeriodDays to GetUserDeactivationInactivityPeriod GetUserDeactivationInactivityNotificationPeriodDays to GetUserDeactivationInactivityNotificationPeriod

Updated user service blackbox test to not multiple the duration by 24 hours again

codecov[bot] commented 5 years ago

Codecov Report

Merging #839 into master will decrease coverage by <.01%. The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #839      +/-   ##
========================================
- Coverage      78%    78%   -0.01%     
========================================
  Files          98     98              
  Lines        9272   9271       -1     
========================================
- Hits         7233   7232       -1     
  Misses       1500   1500              
  Partials      539    539
Impacted Files Coverage Δ
authentication/account/service/user.go 73.8% <100%> (-0.13%) :arrow_down:
configuration/configuration.go 82.42% <100%> (ø) :arrow_up:

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 2a60f45...a86ad20. Read the comment docs.

nurali-techie commented 5 years ago

@xcoulon @alexeykazakov I am not sure if dropping Days from method name is right thing here, WDYT ??

xcoulon commented 5 years ago

@xcoulon @alexeykazakov I am not sure if dropping Days from method name is right thing here, WDYT ??

We can rename those funcs, but let's keep the env var names as they are today so we know we configure in days. But being able to return a duration in seconds during the tests make sense, indeed. As you suggested @nurali-techie, this would bring inconsistency with other funcs which are prefixed with Seconds or Millis, so we should actually remove all notion of units and return a time.Duration, which implies the unit.

alexeykazakov commented 5 years ago

@xcoulon thanks for the review! Yes, now everything should look clean. All methods that return time.duration don't have time units in the method names to avoid confusion. But all the configuration variables specify the unit because they do define time in specific units.

xcoulon commented 5 years ago

@xcoulon thanks for the review! Yes, now everything should look clean. All methods that return time.duration doesn't time units in the method names to avoid confusion. But all the configuration variables specify the unit because they do define time in specific units.

thanks for taking care of those extra changes, @alexeykazakov ! and yes, having such suffixes did not make sense anymore now that the functions return a time.Duration