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

Enhanced 'import_users.py' script using logging module #1240

Open
wants to merge 17 commits into
base: development
Choose a base branch
from

Conversation

ali-ramazani
Copy link
Member

@ali-ramazani ali-ramazani commented Jun 24, 2024

Fixes issue #1053

We enhanced the import_users.py script by integrating Python's logging module to provide detailed and descriptive log messages. These improvements will make the cron.log file significantly more informative, aiding in monitoring the script's execution and diagnosing any issues that arise from crontab scheduled tasks.

@Ndiayem14
Copy link
Contributor

We have upgraded the import_users.py script by incorporating Python's logging module to generate comprehensive and detailed log messages. This enhancement will make the cron.log file much more insightful, facilitating better monitoring of the script's performance and easier troubleshooting of any issues that emerge from the crontab-scheduled tasks.

@WackyWeaver
Copy link
Contributor

This PR seemed to be pretty straightforward. I saw obvious problems with the logging, and I ran unit testing for this. This was not client side, so I saw no need to try to break it on that end. I would approve this PR.

@vungc
Copy link
Contributor

vungc commented Sep 11, 2024

Can you resolve the merge conflicts that exist? @ali-ramazani

@vungc vungc requested review from vungc and ojmakinde September 11, 2024 19:44
vungc
vungc previously requested changes Sep 11, 2024
Copy link
Contributor

@vungc vungc left a comment

Choose a reason for hiding this comment

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

Need to resolve merge conflicts.

@hoerstl hoerstl self-assigned this Nov 6, 2024
@hoerstl hoerstl dismissed vungc’s stale review November 6, 2024 17:11

I resolved all the merge conflicts

@hoerstl
Copy link
Contributor

hoerstl commented Nov 8, 2024

I was running this in a devcontainer and I couldn't check this script with tracy but I looked over it and everything looks to be good. Let's give this one final run before we merge it.

Copy link
Contributor

@WackyWeaver WackyWeaver left a comment

Choose a reason for hiding this comment

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

I approve it, but I would also want someone to try running it with TRACY locally.

Copy link
Contributor

@BrianRamsay BrianRamsay left a comment

Choose a reason for hiding this comment

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

Based on https://docs.python.org/3/library/logging.html#levels, the log levels should be better defined. Log level DEBUG should show all of the output, INFO should give a summary of what the script did (count of users updated/inserted, etc), and WARNING should be when something unexpected happens, or we can't match a user.

We should be able to control the log levels in an argument to the script.

The output should go to stdout, not to a file. Let crontab control where the output goes.

Copy link
Contributor

@BrianRamsay BrianRamsay left a comment

Choose a reason for hiding this comment

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

It doesn't seem like there's an intermediate level of detail, but maybe that's because the first updates don't work right now. For ldap at least I either get only errors, or all of the info output.

app/scripts/import_users.py Outdated Show resolved Hide resolved
Copy link
Contributor

@hoerstl hoerstl left a comment

Choose a reason for hiding this comment

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

I've fully tested the changes I made to the dockerfile and the logging code works perfectly.

build-essential libssl-dev libffi-dev freetds-dev freetds-bin unixodbc-dev tdsodbc

RUN sudo bash -c "echo -e '[FreeTDS]\nDescription=FreeTDS Driver\nDriver=/usr/lib/x86_64-linux-gnu/odbc/libtdsodbc.so\
\nSetup=/usr/lib/x86_64-linux-gnu/odbc/libtdsS.so' >> /etc/odbcinst.ini"
Copy link
Contributor

Choose a reason for hiding this comment

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

I couldn't find a way to break this into two lines while keeping the indentation style so we're just going to have to live with it.

Copy link

View Code Coverage

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.

7 participants