From 7957ac8464282dbb6eac572ac453c29f1998205a Mon Sep 17 00:00:00 2001 From: Bruno Carrez Date: Mon, 27 May 2024 10:46:54 -0400 Subject: [PATCH 1/9] temporarily disable mymila source --- sarc/ldap/acquire.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/sarc/ldap/acquire.py b/sarc/ldap/acquire.py index 77f50fbb..c7fd0945 100644 --- a/sarc/ldap/acquire.py +++ b/sarc/ldap/acquire.py @@ -42,11 +42,12 @@ def run( cache_policy=cache_policy, ) - LD_users = fetch_mymila( - cfg, - LD_users, - cache_policy=cache_policy, - ) + # MyMila scraping is temporary disabled until we have a proper solution. + # LD_users = fetch_mymila( + # cfg, + # LD_users, + # cache_policy=cache_policy, + # ) # For each supervisor or co-supervisor, look for a mila_email_username # matching the display name. If None has been found, the previous value remains From 999f8c636943a2c46a3323700e33d42553eea802 Mon Sep 17 00:00:00 2001 From: Bruno Carrez Date: Mon, 27 May 2024 10:58:09 -0400 Subject: [PATCH 2/9] cleaner "NotImplementedException" handling for `fetch_mymila` call --- sarc/ldap/acquire.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/sarc/ldap/acquire.py b/sarc/ldap/acquire.py index c7fd0945..6d606fe0 100644 --- a/sarc/ldap/acquire.py +++ b/sarc/ldap/acquire.py @@ -42,12 +42,16 @@ def run( cache_policy=cache_policy, ) - # MyMila scraping is temporary disabled until we have a proper solution. - # LD_users = fetch_mymila( - # cfg, - # LD_users, - # cache_policy=cache_policy, - # ) + # MyMila scraping "NotImplementedError" is temporary ignored until we have a working fetching implementation, + # or a working workaround using CSV cache. + with using_trace( + "sarc.ldap.acquire", "fetch_mymila", exception_types=(NotImplementedError,) + ) as span: + LD_users = fetch_mymila( + cfg, + LD_users, + cache_policy=cache_policy, + ) # For each supervisor or co-supervisor, look for a mila_email_username # matching the display name. If None has been found, the previous value remains From e342244647e96d9e741613d3ab48d1e378506632 Mon Sep 17 00:00:00 2001 From: Bruno Carrez Date: Mon, 27 May 2024 11:06:44 -0400 Subject: [PATCH 3/9] automate daily LDAP scraping --- scripts/systemd/scrapers.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/systemd/scrapers.sh b/scripts/systemd/scrapers.sh index ffd6d91c..6f9690c7 100755 --- a/scripts/systemd/scrapers.sh +++ b/scripts/systemd/scrapers.sh @@ -2,4 +2,7 @@ SCRIPT=$(readlink -f "$0") SCRIPTPATH=$(dirname "$SCRIPT") cd $SCRIPTPATH/../../ +# scraping jobs sudo -u sarc SARC_MODE=scraping SARC_CONFIG=$SCRIPTPATH/../../config/sarc-prod.json ../.local/bin/poetry run sarc acquire jobs -c narval cedar beluga graham mila -d auto +# scraping users +sudo -u sarc SARC_MODE=scraping SARC_CONFIG=$SCRIPTPATH/../../config/sarc-prod.json ../.local/bin/poetry run sarc acquire users From 93d857f35006decdd22fc029813791d862a7ccde Mon Sep 17 00:00:00 2001 From: Bruno Carrez Date: Mon, 27 May 2024 13:55:32 -0400 Subject: [PATCH 4/9] fix tests --- .../cli/acquire/test_acquire_users.py | 40 ++++++++++++------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/tests/functional/cli/acquire/test_acquire_users.py b/tests/functional/cli/acquire/test_acquire_users.py index 2c8ac9a6..5613c933 100644 --- a/tests/functional/cli/acquire/test_acquire_users.py +++ b/tests/functional/cli/acquire/test_acquire_users.py @@ -124,17 +124,22 @@ def test_acquire_users(cli_main, patch_return_values, mock_file, captrace): # as everything goes well without corner cases. # We will test logging in test_acquire_users_prompt below. spans = captrace.get_finished_spans() - assert len(spans) == 1 - assert spans[0].name == "match_drac_to_mila_accounts" + assert len(spans) == 2 + + assert spans[0].name == "fetch_mymila" assert spans[0].status.status_code == StatusCode.OK - assert len(spans[0].events) == 4 + assert len(spans[0].events) == 0 + + assert spans[1].name == "match_drac_to_mila_accounts" + assert spans[1].status.status_code == StatusCode.OK + assert len(spans[1].events) == 4 assert ( - spans[0].events[0].name + spans[1].events[0].name == "Loading mila_ldap, drac_roles and drac_members from files ..." ) - assert spans[0].events[1].name == "Loading matching config from file ..." - assert spans[0].events[2].name == "Matching DRAC/CC to mila accounts ..." - assert spans[0].events[3].name == "Committing matches to database ..." + assert spans[1].events[1].name == "Loading matching config from file ..." + assert spans[1].events[2].name == "Matching DRAC/CC to mila accounts ..." + assert spans[1].events[3].name == "Committing matches to database ..." @pytest.mark.parametrize( @@ -398,15 +403,20 @@ def test_acquire_users_prompt( # Check traces spans = captrace.get_finished_spans() - assert len(spans) == 1 - assert spans[0].name == "match_drac_to_mila_accounts" + assert len(spans) == 2 + + assert spans[0].name == "fetch_mymila" assert spans[0].status.status_code == StatusCode.OK - assert len(spans[0].events) == 5 + assert len(spans[0].events) == 0 + + assert spans[1].name == "match_drac_to_mila_accounts" + assert spans[1].status.status_code == StatusCode.OK + assert len(spans[1].events) == 5 assert ( - spans[0].events[0].name + spans[1].events[0].name == "Loading mila_ldap, drac_roles and drac_members from files ..." ) - assert spans[0].events[1].name == "Loading matching config from file ..." - assert spans[0].events[2].name == "Matching DRAC/CC to mila accounts ..." - assert spans[0].events[3].name == "Committing matches to database ..." - assert spans[0].events[4].name == "Saving 1 manual matches ..." + assert spans[1].events[1].name == "Loading matching config from file ..." + assert spans[1].events[2].name == "Matching DRAC/CC to mila accounts ..." + assert spans[1].events[3].name == "Committing matches to database ..." + assert spans[1].events[4].name == "Saving 1 manual matches ..." From 138c3ab2e7edcd3dae315b2d268756e138bdd0f7 Mon Sep 17 00:00:00 2001 From: Bruno Carrez Date: Mon, 3 Jun 2024 18:11:38 -0400 Subject: [PATCH 5/9] Add test for users versions count --- tests/functional/ldap/test_acquire_ldap.py | 65 +++++++++++++++++++++- 1 file changed, 64 insertions(+), 1 deletion(-) diff --git a/tests/functional/ldap/test_acquire_ldap.py b/tests/functional/ldap/test_acquire_ldap.py index afc19ba4..262bf276 100644 --- a/tests/functional/ldap/test_acquire_ldap.py +++ b/tests/functional/ldap/test_acquire_ldap.py @@ -5,7 +5,7 @@ import sarc.account_matching.make_matches import sarc.ldap.acquire -from sarc.ldap.api import get_user +from sarc.ldap.api import get_user,get_users @pytest.mark.usefixtures("empty_read_write_db") @@ -67,6 +67,69 @@ def test_acquire_ldap(patch_return_values, mock_file): js_user = get_user(drac_account_username="ms@hotmail.com") assert js_user is None +@pytest.mark.usefixtures("empty_read_write_db") +def test_acquire_ldap_revision_change(patch_return_values, mock_file): + """ + Test two LDAP acquisition, with a change in the LDAP data. + This should result in a new record in the database. + Then, one third acquisition, with no change in the LDAP data. + This should result in no change in the database. + """ + nbr_users = 3 + + patch_return_values( + { + "sarc.ldap.read_mila_ldap.query_ldap": fake_raw_ldap_data(nbr_users), + "sarc.ldap.mymila.query_mymila_csv": [], + } + ) + + # Patch the built-in `open()` function for each file path + with patch("builtins.open", side_effect=mock_file): + sarc.ldap.acquire.run() + + # inspect database to check the number of records + users = get_users() + nb_users_1 = len(users) + nb_users_1 == nbr_users + + # re-acquire the same data + with patch("builtins.open", side_effect=mock_file): + sarc.ldap.acquire.run() + + # inspect database to check the number of records + users = get_users() + assert len(users) == nb_users_1 + + # change fake data + patch_return_values( + { + "sarc.ldap.read_mila_ldap.query_ldap": fake_raw_ldap_data( + nbr_users, + hardcoded_values_by_user={ + 2: { # The first user who is not a prof is the one with index 2 + "supervisor": "new_supervisor@mila.quebec" + } + } + } + ) + + # re-acquire the new data + with patch("builtins.open", side_effect=mock_file): + sarc.ldap.acquire.run() + + # inspect database to check the number of records + users = get_users() + assert len(users) == nb_users_1+1 + + # re-acquire the same data + with patch("builtins.open", side_effect=mock_file): + sarc.ldap.acquire.run() + + # inspect database to check the number of records + users = get_users() + assert len(users) == nb_users_1+1 + @pytest.mark.usefixtures("empty_read_write_db") def test_merge_ldap_and_mymila(patch_return_values, mock_file): From a48625fc695403a864a0a4157b3699cc45e6b14c Mon Sep 17 00:00:00 2001 From: Bruno Carrez Date: Mon, 3 Jun 2024 18:13:42 -0400 Subject: [PATCH 6/9] lint --- tests/functional/ldap/test_acquire_ldap.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/tests/functional/ldap/test_acquire_ldap.py b/tests/functional/ldap/test_acquire_ldap.py index 262bf276..c8fe3f6a 100644 --- a/tests/functional/ldap/test_acquire_ldap.py +++ b/tests/functional/ldap/test_acquire_ldap.py @@ -5,7 +5,7 @@ import sarc.account_matching.make_matches import sarc.ldap.acquire -from sarc.ldap.api import get_user,get_users +from sarc.ldap.api import get_user, get_users @pytest.mark.usefixtures("empty_read_write_db") @@ -67,6 +67,7 @@ def test_acquire_ldap(patch_return_values, mock_file): js_user = get_user(drac_account_username="ms@hotmail.com") assert js_user is None + @pytest.mark.usefixtures("empty_read_write_db") def test_acquire_ldap_revision_change(patch_return_values, mock_file): """ @@ -91,7 +92,7 @@ def test_acquire_ldap_revision_change(patch_return_values, mock_file): # inspect database to check the number of records users = get_users() nb_users_1 = len(users) - nb_users_1 == nbr_users + nb_users_1 == nbr_users # re-acquire the same data with patch("builtins.open", side_effect=mock_file): @@ -105,12 +106,13 @@ def test_acquire_ldap_revision_change(patch_return_values, mock_file): patch_return_values( { "sarc.ldap.read_mila_ldap.query_ldap": fake_raw_ldap_data( - nbr_users, - hardcoded_values_by_user={ + nbr_users, + hardcoded_values_by_user={ 2: { # The first user who is not a prof is the one with index 2 "supervisor": "new_supervisor@mila.quebec" } - } + }, + ) } ) @@ -120,7 +122,7 @@ def test_acquire_ldap_revision_change(patch_return_values, mock_file): # inspect database to check the number of records users = get_users() - assert len(users) == nb_users_1+1 + assert len(users) == nb_users_1 + 1 # re-acquire the same data with patch("builtins.open", side_effect=mock_file): @@ -128,7 +130,7 @@ def test_acquire_ldap_revision_change(patch_return_values, mock_file): # inspect database to check the number of records users = get_users() - assert len(users) == nb_users_1+1 + assert len(users) == nb_users_1 + 1 @pytest.mark.usefixtures("empty_read_write_db") From 0333861389437b3c44495d65f314e489feb6c2b0 Mon Sep 17 00:00:00 2001 From: Bruno Carrez Date: Mon, 3 Jun 2024 18:19:39 -0400 Subject: [PATCH 7/9] update test --- tests/functional/ldap/test_acquire_ldap.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/functional/ldap/test_acquire_ldap.py b/tests/functional/ldap/test_acquire_ldap.py index c8fe3f6a..5edce547 100644 --- a/tests/functional/ldap/test_acquire_ldap.py +++ b/tests/functional/ldap/test_acquire_ldap.py @@ -90,7 +90,7 @@ def test_acquire_ldap_revision_change(patch_return_values, mock_file): sarc.ldap.acquire.run() # inspect database to check the number of records - users = get_users() + users = get_users(latest=False) nb_users_1 = len(users) nb_users_1 == nbr_users @@ -99,7 +99,7 @@ def test_acquire_ldap_revision_change(patch_return_values, mock_file): sarc.ldap.acquire.run() # inspect database to check the number of records - users = get_users() + users = get_users(latest=False) assert len(users) == nb_users_1 # change fake data @@ -121,7 +121,7 @@ def test_acquire_ldap_revision_change(patch_return_values, mock_file): sarc.ldap.acquire.run() # inspect database to check the number of records - users = get_users() + users = get_users(latest=False) assert len(users) == nb_users_1 + 1 # re-acquire the same data @@ -129,7 +129,7 @@ def test_acquire_ldap_revision_change(patch_return_values, mock_file): sarc.ldap.acquire.run() # inspect database to check the number of records - users = get_users() + users = get_users(latest=False) assert len(users) == nb_users_1 + 1 From c157acfcf02a07fe63a773f9ce8207c05db83668 Mon Sep 17 00:00:00 2001 From: Bruno Carrez Date: Mon, 3 Jun 2024 18:32:26 -0400 Subject: [PATCH 8/9] ignore "record_start" and "record_end" fields for revision comparison in `has_changed` method --- sarc/ldap/revision.py | 10 +++++++++- tests/functional/ldap/test_acquire_ldap.py | 4 ++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/sarc/ldap/revision.py b/sarc/ldap/revision.py index 82493d98..849552e2 100644 --- a/sarc/ldap/revision.py +++ b/sarc/ldap/revision.py @@ -27,7 +27,15 @@ def is_date_missing(date): return date is None or date == DEFAULT_DATE -def has_changed(user_db, user_latest, excluded=("_id",)): +def has_changed( + user_db, + user_latest, + excluded=( + "_id", + "record_start", + "record_end", + ), +): keys = set(list(user_db.keys()) + list(user_latest.keys())) for k in keys: diff --git a/tests/functional/ldap/test_acquire_ldap.py b/tests/functional/ldap/test_acquire_ldap.py index 5edce547..dd7ad2b6 100644 --- a/tests/functional/ldap/test_acquire_ldap.py +++ b/tests/functional/ldap/test_acquire_ldap.py @@ -90,6 +90,7 @@ def test_acquire_ldap_revision_change(patch_return_values, mock_file): sarc.ldap.acquire.run() # inspect database to check the number of records + # should be nbr_users users = get_users(latest=False) nb_users_1 = len(users) nb_users_1 == nbr_users @@ -99,6 +100,7 @@ def test_acquire_ldap_revision_change(patch_return_values, mock_file): sarc.ldap.acquire.run() # inspect database to check the number of records + # should be the same users = get_users(latest=False) assert len(users) == nb_users_1 @@ -121,6 +123,7 @@ def test_acquire_ldap_revision_change(patch_return_values, mock_file): sarc.ldap.acquire.run() # inspect database to check the number of records + # should be incremented by 1 users = get_users(latest=False) assert len(users) == nb_users_1 + 1 @@ -129,6 +132,7 @@ def test_acquire_ldap_revision_change(patch_return_values, mock_file): sarc.ldap.acquire.run() # inspect database to check the number of records + # should be the same users = get_users(latest=False) assert len(users) == nb_users_1 + 1 From f65fbb0ca4126ab2f202465de16b003d798d56ed Mon Sep 17 00:00:00 2001 From: Bruno Carrez Date: Mon, 3 Jun 2024 18:44:00 -0400 Subject: [PATCH 9/9] fix test typo --- tests/functional/ldap/test_acquire_ldap.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/functional/ldap/test_acquire_ldap.py b/tests/functional/ldap/test_acquire_ldap.py index dd7ad2b6..4173ef5d 100644 --- a/tests/functional/ldap/test_acquire_ldap.py +++ b/tests/functional/ldap/test_acquire_ldap.py @@ -93,7 +93,7 @@ def test_acquire_ldap_revision_change(patch_return_values, mock_file): # should be nbr_users users = get_users(latest=False) nb_users_1 = len(users) - nb_users_1 == nbr_users + assert nb_users_1 == nbr_users # re-acquire the same data with patch("builtins.open", side_effect=mock_file):