refactor: migrate `hooks` configuration feature to use python-gitlab library #635

TigreModerata · 2023-11-04T17:09:58Z

Removed ProjectHook methods from gitlab Projects;
Changed HooksProcessor to use python-gitlab methods;
Added a test_hooks_processor file to acceptance tests to check if new hooks processor can create, update, delete

closes #624

Changed hooks processor to use python-gitlab; Removed hook methods from projects; Added test file to check if new processor can delete, update, create

codecov · 2023-11-04T17:11:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d309d28) 83.27% compared to head (f3276a8) 84.21%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #635      +/-   ##
==========================================
+ Coverage   83.27%   84.21%   +0.94%     
==========================================
  Files          70       70              
  Lines        2744     2743       -1     
==========================================
+ Hits         2285     2310      +25     
+ Misses        459      433      -26

Files	Coverage Δ
gitlabform/gitlab/projects.py	`57.14% <ø> (+5.52%)`	⬆️
gitlabform/processors/project/hooks_processor.py	`97.14% <100.00%> (+62.14%)`	⬆️

... and 1 file with indirect coverage changes

amimas

@TigreModerata - Thank you so much for this PR. It's appreciated and looks pretty good. A few feedbacks from me, but might be best to have @gdubicki to review it as well.

Let's apply types to variables (ie. myvar: str = "hello").

I had no idea the project didn't already have acceptance test for hooks. Thank you for adding one 😍 . Could you simplify the test file though? Take a look at the other test files to see how they are written. Here's what I can think about off the top of my head:

Create separate function/method for different test cases (i.e. create, delete, update, etc)
Use existing fixtures in your test functions/methods; this way you necessary group or project will be created for you already. Existing test files might be helpful as a reference.
Move the gitlabform config into relevant test methods

gitlabform/processors/project/hooks_processor.py

tests/acceptance/standard/test_hooks_processor.py

TigreModerata · 2023-11-04T21:47:17Z

Thank you for the feedback! Will get right on it.

TigreModerata · 2023-11-05T18:02:42Z

Had some mishaps, but it should be better now.
Is there a better way to handle deletes, without having to time.sleep? I have a 6 second sleep because at times even 5 wasn't enough (running gitlab in docker)...

gitlabform/processors/project/hooks_processor.py

tests/acceptance/standard/test_hooks_processor.py

gitlabform/processors/project/hooks_processor.py

amimas · 2023-11-06T05:46:42Z

Is there a better way to handle deletes, without having to time.sleep? I have a 6 second sleep because at times even 5 wasn't enough (running gitlab in docker)...

Which delete are you referring to? Deleting hooks?

TigreModerata · 2023-11-06T19:26:25Z

Yes, deleting hooks here - but that won't matter once I use the fixtures you mention above, rather than hardcode my configs.
I just wondered in general, about any of the objects, especially when you have to delete + create to update (say protectedbranches, since I was looking at that too)...

TigreModerata · 2023-11-07T03:13:18Z

I can't get the failing test to pass, I don't know what's wrong.
Test file should be better now, using fixtures.

amimas

Thanks again for all the updates and keep digging into this. Looking a lot cleaner now.

gitlabform/processors/project/hooks_processor.py

tests/acceptance/standard/test_hooks_processor.py

tests/acceptance/conftest.py

tests/acceptance/standard/test_hooks_processor.py

tests/acceptance/conftest.py

TigreModerata · 2023-11-07T16:29:33Z

Type still not happy with ListRESTObjects for list of Project Hooks...
test_files sometimes fails on here, but seems fine "on my machine"

TigreModerata · 2023-11-09T16:06:00Z

Made the changes and resubmitted.

amimas

Latest change looks good to me. Would be great to get some feedback from @gdubicki when he has some time.

amimas · 2023-11-16T20:03:56Z

Looks good 👍

amimas · 2023-11-24T02:04:24Z

@gdubicki -- should we merge this? Will you have time for a sanity check?

gdubicki · 2023-12-03T11:52:22Z

gitlabform/processors/project/hooks_processor.py

-                        project_and_group, hook, configuration["hooks"][hook]
+                    created_hook: RESTObject = project.hooks.create(hook_config)
+                    debug("Created hook '%s'", created_hook)
+                elif gitlab_hook and gitlab_hook.asdict() != hook_config:


Hi @TigreModerata!

Thank you for the contribution. :)

However I suspected that this condition will always be true because in hook_config we have only a few fields from a hook that we get from the API (gitlab_hook.asdict()), f.e. we don't have the field id.

And it turned out to be true - I modified a test test_hooks_update to do the update 2 times and in the second time the update is done again.

When I runned it with pytest tests/acceptance/standard/test_hooks.py -k 'test_hooks_create or test_hooks_update'.

First time logs excerpt:

Processing hooks... http://localhost:80 "GET /api/v4/projects/gitlabform_group_papaya%2Fgitlabform_project_stony HTTP/1.1" 200 None http://localhost:80 "GET /api/v4/projects/8/hooks HTTP/1.1" 200 None Changing existing hook 'http://hooks/gitlabform_hook_festival.org' http://localhost:80 "PUT /api/v4/projects/8/hooks/9 HTTP/1.1" 200 None Changed hook to '{'id': 9, 'url': 'http://hooks/gitlabform_hook_festival.org', 'created_at': '2023-12-03T11:41:51.849Z', 'push_events': True, 'tag_push_events': False, 'merge_requests_events': False, 'repository_update_events': False, 'enable_ssl_verification': True, 'alert_status': 'executable', 'disabled_until': None, 'url_variables': [], 'project_id': 8, 'issues_events': False, 'confidential_issues_events': False, 'note_events': True, 'confidential_note_events': None, 'pipeline_events': False, 'wiki_page_events': False, 'deployment_events': False, 'job_events': True, 'releases_events': False, 'push_events_branch_filter': None, 'emoji_events': False}' Changing existing hook 'http://hooks/gitlabform_hook_tiring.org' http://localhost:80 "PUT /api/v4/projects/8/hooks/10 HTTP/1.1" 200 None Changed hook to '{'id': 10, 'url': 'http://hooks/gitlabform_hook_tiring.org', 'created_at': '2023-12-03T11:41:51.907Z', 'push_events': False, 'tag_push_events': False, 'merge_requests_events': False, 'repository_update_events': False, 'enable_ssl_verification': True, 'alert_status': 'executable', 'disabled_until': None, 'url_variables': [], 'project_id': 8, 'issues_events': False, 'confidential_issues_events': False, 'note_events': True, 'confidential_note_events': None, 'pipeline_events': False, 'wiki_page_events': False, 'deployment_events': False, 'job_events': False, 'releases_events': False, 'push_events_branch_filter': None, 'emoji_events': False}' * (1/1) FINISHED Processing project: gitlabform_group_papaya/gitlabform_project_stony

Second time logs excerpt:

Processing hooks... http://localhost:80 "GET /api/v4/projects/gitlabform_group_papaya%2Fgitlabform_project_stony HTTP/1.1" 200 None http://localhost:80 "GET /api/v4/projects/8/hooks HTTP/1.1" 200 None Changing existing hook 'http://hooks/gitlabform_hook_festival.org' http://localhost:80 "PUT /api/v4/projects/8/hooks/9 HTTP/1.1" 200 None Changed hook to '{'id': 9, 'url': 'http://hooks/gitlabform_hook_festival.org', 'created_at': '2023-12-03T11:41:51.849Z', 'push_events': True, 'tag_push_events': False, 'merge_requests_events': False, 'repository_update_events': False, 'enable_ssl_verification': True, 'alert_status': 'executable', 'disabled_until': None, 'url_variables': [], 'project_id': 8, 'issues_events': False, 'confidential_issues_events': False, 'note_events': True, 'confidential_note_events': None, 'pipeline_events': False, 'wiki_page_events': False, 'deployment_events': False, 'job_events': True, 'releases_events': False, 'push_events_branch_filter': None, 'emoji_events': False}' Changing existing hook 'http://hooks/gitlabform_hook_tiring.org' http://localhost:80 "PUT /api/v4/projects/8/hooks/10 HTTP/1.1" 200 None Changed hook to '{'id': 10, 'url': 'http://hooks/gitlabform_hook_tiring.org', 'created_at': '2023-12-03T11:41:51.907Z', 'push_events': False, 'tag_push_events': False, 'merge_requests_events': False, 'repository_update_events': False, 'enable_ssl_verification': True, 'alert_status': 'executable', 'disabled_until': None, 'url_variables': [], 'project_id': 8, 'issues_events': False, 'confidential_issues_events': False, 'note_events': True, 'confidential_note_events': None, 'pipeline_events': False, 'wiki_page_events': False, 'deployment_events': False, 'job_events': False, 'releases_events': False, 'push_events_branch_filter': None, 'emoji_events': False}' * (1/1) FINISHED Processing project: gitlabform_group_papaya/gitlabform_project_stony

This is not great as it lowers the performance of a run and may cause unnecessary audit events in the projects on each run ("Hook ... has been updated").

I have pushed my commit to show the problem here d8b84e1.

@TigreModerata - no rush but just wondering if you will be able to look into this feedback? I feel this PR is so close to being merged🤞

It's a good callout from @gdubicki . I missed this.

Hi both, sorry for the late reply, I was traveling. I will have a look today, thanks a lot for the feedback!

TigreModerata · 2023-12-09T15:28:52Z

OK I think I fixed it. I included a test, reading the debug logs to check if indeed an identical config is not getting updated.

amimas · 2023-12-10T05:35:12Z

OK I think I fixed it. I included a test, reading the debug logs to check if indeed an identical config is not getting updated.

Thank you @TigreModerata . appreciate it.

@gdubicki - could we get one more review please?

amimas

Looks good. Thanks again for this PR.

TigreModerata · 2024-01-04T13:26:54Z

Cool, sorry again for all the missteps!

jimisola · 2024-01-04T13:49:01Z

Cool, sorry again for all the missteps!

Absolutely no reason to say sorry. Thank you for giving your time and contributing.

migrated hooks processor to gitlab

2f22a51

Changed hooks processor to use python-gitlab; Removed hook methods from projects; Added test file to check if new processor can delete, update, create

TigreModerata had a problem deploying to Integrate Pull Request November 4, 2023 17:10 — with GitHub Actions Failure

amimas requested changes Nov 4, 2023

View reviewed changes

gitlabform/processors/project/hooks_processor.py Outdated Show resolved Hide resolved

gitlabform/processors/project/hooks_processor.py Outdated Show resolved Hide resolved

tests/acceptance/standard/test_hooks_processor.py Outdated Show resolved Hide resolved

amimas changed the title ~~#624~~ refactor: migrate hooks configuration feature to use python-gitlab library Nov 4, 2023

TigreModerata had a problem deploying to Integrate Pull Request November 5, 2023 13:09 — with GitHub Actions Failure

TigreModerata had a problem deploying to Integrate Pull Request November 5, 2023 15:25 — with GitHub Actions Failure

TigreModerata had a problem deploying to Integrate Pull Request November 5, 2023 15:39 — with GitHub Actions Failure

TigreModerata had a problem deploying to Integrate Pull Request November 5, 2023 15:59 — with GitHub Actions Failure

TigreModerata had a problem deploying to Integrate Pull Request November 5, 2023 16:27 — with GitHub Actions Failure

TigreModerata had a problem deploying to Integrate Pull Request November 5, 2023 17:42 — with GitHub Actions Failure

TigreModerata had a problem deploying to Integrate Pull Request November 5, 2023 17:55 — with GitHub Actions Failure

amimas requested changes Nov 6, 2023

View reviewed changes

amimas requested a review from gdubicki November 6, 2023 05:46

TigreModerata had a problem deploying to Integrate Pull Request November 6, 2023 19:14 — with GitHub Actions Failure

TigreModerata had a problem deploying to Integrate Pull Request November 6, 2023 23:48 — with GitHub Actions Failure

TigreModerata had a problem deploying to Integrate Pull Request November 6, 2023 23:56 — with GitHub Actions Failure

TigreModerata had a problem deploying to Integrate Pull Request November 7, 2023 01:23 — with GitHub Actions Failure

TigreModerata had a problem deploying to Integrate Pull Request November 7, 2023 02:22 — with GitHub Actions Failure

TigreModerata had a problem deploying to Integrate Pull Request November 7, 2023 02:35 — with GitHub Actions Failure

amimas requested changes Nov 7, 2023

View reviewed changes

TigreModerata had a problem deploying to Integrate Pull Request November 7, 2023 16:12 — with GitHub Actions Failure

TigreModerata had a problem deploying to Integrate Pull Request November 7, 2023 16:50 — with GitHub Actions Failure

TigreModerata had a problem deploying to Integrate Pull Request November 7, 2023 17:04 — with GitHub Actions Failure

TigreModerata had a problem deploying to Integrate Pull Request November 9, 2023 15:05 — with GitHub Actions Failure

amimas reviewed Nov 10, 2023

View reviewed changes

amimas temporarily deployed to Integrate Pull Request November 14, 2023 21:57 — with GitHub Actions Inactive

amimas temporarily deployed to Integrate Pull Request November 15, 2023 00:08 — with GitHub Actions Inactive

TigreModerata had a problem deploying to Integrate Pull Request November 15, 2023 14:59 — with GitHub Actions Failure

TigreModerata temporarily deployed to Integrate Pull Request November 15, 2023 15:17 — with GitHub Actions Inactive

gdubicki reviewed Dec 3, 2023

View reviewed changes

TigreModerata had a problem deploying to Integrate Pull Request December 9, 2023 14:53 — with GitHub Actions Failure

Migrated hooks processing to python-gitlab

6920c98

TigreModerata force-pushed the #624 branch from 3d5930e to 6920c98 Compare December 9, 2023 15:22

TigreModerata had a problem deploying to Integrate Pull Request December 9, 2023 15:22 — with GitHub Actions Failure

Merge branch 'main' into gitlabform#624

af3b6b3

TigreModerata temporarily deployed to Integrate Pull Request December 9, 2023 15:23 — with GitHub Actions Inactive

Merge branch 'main' into gitlabform#624

dc2bb5d

TigreModerata temporarily deployed to Integrate Pull Request December 14, 2023 11:23 — with GitHub Actions Inactive

amimas requested a review from gdubicki December 15, 2023 14:39

Merge branch 'main' into gitlabform#624

f3276a8

amimas temporarily deployed to Integrate Pull Request January 3, 2024 07:05 — with GitHub Actions Inactive

amimas approved these changes Jan 4, 2024

View reviewed changes

amimas merged commit b5513de into gitlabform:main Jan 4, 2024
23 checks passed

TigreModerata deleted the #624 branch January 4, 2024 13:27

This was referenced Feb 19, 2024

3.9.0 appears to break while setting a project webhook with a token #685

Closed

fix: hooks processing fails when config includes token #693

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: migrate `hooks` configuration feature to use python-gitlab library #635

refactor: migrate `hooks` configuration feature to use python-gitlab library #635

TigreModerata commented Nov 4, 2023 •

edited by amimas

Loading

codecov bot commented Nov 4, 2023 •

edited

Loading

amimas left a comment

TigreModerata commented Nov 4, 2023

TigreModerata commented Nov 5, 2023

amimas commented Nov 6, 2023

TigreModerata commented Nov 6, 2023

TigreModerata commented Nov 7, 2023

amimas left a comment

TigreModerata commented Nov 7, 2023

TigreModerata commented Nov 9, 2023

amimas left a comment

amimas commented Nov 16, 2023

amimas commented Nov 24, 2023

gdubicki Dec 3, 2023

amimas Dec 7, 2023

TigreModerata Dec 9, 2023

TigreModerata commented Dec 9, 2023

amimas commented Dec 10, 2023

amimas left a comment

TigreModerata commented Jan 4, 2024

jimisola commented Jan 4, 2024

refactor: migrate hooks configuration feature to use python-gitlab library #635

refactor: migrate hooks configuration feature to use python-gitlab library #635

Conversation

TigreModerata commented Nov 4, 2023 • edited by amimas Loading

codecov bot commented Nov 4, 2023 • edited Loading

Codecov Report

amimas left a comment

Choose a reason for hiding this comment

TigreModerata commented Nov 4, 2023

TigreModerata commented Nov 5, 2023

amimas commented Nov 6, 2023

TigreModerata commented Nov 6, 2023

TigreModerata commented Nov 7, 2023

amimas left a comment

Choose a reason for hiding this comment

TigreModerata commented Nov 7, 2023

TigreModerata commented Nov 9, 2023

amimas left a comment

Choose a reason for hiding this comment

amimas commented Nov 16, 2023

amimas commented Nov 24, 2023

gdubicki Dec 3, 2023

Choose a reason for hiding this comment

amimas Dec 7, 2023

Choose a reason for hiding this comment

TigreModerata Dec 9, 2023

Choose a reason for hiding this comment

TigreModerata commented Dec 9, 2023

amimas commented Dec 10, 2023

amimas left a comment

Choose a reason for hiding this comment

TigreModerata commented Jan 4, 2024

jimisola commented Jan 4, 2024

refactor: migrate `hooks` configuration feature to use python-gitlab library #635

refactor: migrate `hooks` configuration feature to use python-gitlab library #635

TigreModerata commented Nov 4, 2023 •

edited by amimas

Loading

codecov bot commented Nov 4, 2023 •

edited

Loading