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

Add last used function #159

Merged
merged 6 commits into from
Mar 6, 2019
Merged

Add last used function #159

merged 6 commits into from
Mar 6, 2019

Conversation

TonyLianLong
Copy link
Member

I added a last used function since it would be useful if we can figure out who is the last user in cases such as (ocf/utils#96).

We can use last_used(...)["end"] == None to determine whether the user is currently using the machine.

Copy link
Member

@abizer abizer left a comment

Choose a reason for hiding this comment

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

this should really be a function in ocflib.lab.stats

@dkess
Copy link
Member

dkess commented Mar 4, 2019

ocflib.lab.stats uses the anonymous MySQL user, whereas this uses credentials, so I think it's OK for this to be in a separate module.

@abizer
Copy link
Member

abizer commented Mar 4, 2019

the functions in that module can/should have the ability to take credentials. splitting this functionality out like this without good reason breaks the logical design of ocflib.

@TonyLianLong
Copy link
Member Author

TonyLianLong commented Mar 4, 2019

the functions in that module can/should have the ability to take credentials. splitting this functionality out like this without good reason breaks the logical design of ocflib.

So probably I can put the code into that module and refractor a little bit so that when specified with some arguments it does not connect with anonymous username? This is not so easy though since in what step should we connect to the database? @abizer

@dkess
Copy link
Member

dkess commented Mar 4, 2019

Look at how the other functions call get_connection. It's actually a partial function, which means you can call get_connection(user='username', password='aaaaa', db='db'), etc.

@TonyLianLong
Copy link
Member Author

Now I put those functions into stats.py with some slight modifications.

@TonyLianLong TonyLianLong requested a review from abizer March 4, 2019 21:38
Copy link
Member

@dkess dkess left a comment

Choose a reason for hiding this comment

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

lgtm, just some small nits

ocflib/lab/stats.py Outdated Show resolved Hide resolved
ocflib/lab/stats.py Outdated Show resolved Hide resolved
ocflib/lab/stats.py Outdated Show resolved Hide resolved
@dkess
Copy link
Member

dkess commented Mar 5, 2019

This lgtm, to fix the last errors you should make sure to run make install-hooks on your local environment. Then running git commit will automatically fix the style errors. You can also run pre-commit manually to fix the errors.

@TonyLianLong TonyLianLong merged commit e427a45 into ocf:master Mar 6, 2019
ocflib/lab/stats.py Show resolved Hide resolved


def last_used(host):
with open('/etc/ocfstats-ro.passwd', 'r') as fin:
Copy link
Member

Choose a reason for hiding this comment

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

ocflib shouldn't concern itself with getting credentials itself - it should be able to operate regardless of where the credentials are, i.e. these functions should operate by accepting an authenticated context object, e.g. shorturls.py, mail.py rather than by hardcoding the location of a file on disk, especially when this file isn't universally available.

ocflib/lab/stats.py Show resolved Hide resolved
ocflib/lab/stats.py Show resolved Hide resolved
@abizer
Copy link
Member

abizer commented Mar 7, 2019

There are still issues with this PR that ought to be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants