fix: hooks processing fails when config includes token #693

amimas · 2024-02-25T04:57:03Z

The hooks processor was recently refactored in #635 . One of the change was to inspect and only update a hook when it is needed so that unnecessary api call/update is avoided. This was done by comparing hooks config with data from gitlab about project hooks.

The above fails when a hook config includes a token attribute. It's treated as secret and so it is not available in the gitlab data. The comparison fails and gitlabform produces error processing the project.

This PR fixes this issue by adding additional logic for hooks config with token. In this case an update is always done. Otherwise continue with existing logic of updating only if necessary. Updated test cases to validate it.

Fixes #685

codecov · 2024-02-25T04:57:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.39%. Comparing base (30d9540) to head (f50a044).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #693      +/-   ##
==========================================
+ Coverage   84.31%   84.39%   +0.08%     
==========================================
  Files          69       69              
  Lines        2747     2749       +2     
==========================================
+ Hits         2316     2320       +4     
+ Misses        431      429       -2

Files	Coverage Δ
gitlabform/processors/project/hooks_processor.py	`100.00% <100.00%> (+2.43%)`	⬆️

... and 1 file with indirect coverage changes

amimas · 2024-02-25T07:48:18Z

@TigreModerata - you helped with hooks processor recently on couple occasions. Wondering if you would be able to help with a review of this PR. Thanks!

TigreModerata · 2024-02-26T13:51:27Z

@TigreModerata - you helped with hooks processor recently on couple occasions. Wondering if you would be able to help with a review of this PR. Thanks!

Yes, sure, I'm sorry I was slow with my PR (very busy last week, unexpectedly). Good thing you did it anyway, because I wasn't going to add extra logic, rather I was just going to revert back to updating all hooks no matter what.

TigreModerata

I would suggest reducing the number of nested if's: usually in python I think more than 3 steps is considered too much.
As for the situation when a token is included in the config: one possibility would be to add an empty-string token to the gl_hook by hand, so that the rest of the processor can continue and compare the config as before. I think it improves readability and makes the nested-if's simplification easy. I don't see any obvious objections to this right now, but maybe you can correct me.

TigreModerata

I tried to include my code snippets here, but it's unreadable if I suggest code one line at a time (I can't see how to do multiline suggestions).

TigreModerata · 2024-02-26T18:39:45Z

gitlabform/processors/project/hooks_processor.py

-                    debug("Changed hook to '%s'", changed_hook)
-                elif hook_id and not any(diffs):
-                    debug(f"Hook {hook} remains unchanged")
+                    debug(f"Created hook: {created_hook}")


Suggested change

debug(f"Created hook: {created_hook}")

debug(f"Created hook: {created_hook}")

continue

'continue' to skip right ahead to the next element in the for loop.

TigreModerata · 2024-02-26T18:40:51Z

gitlabform/processors/project/hooks_processor.py

-                elif hook_id and not any(diffs):
-                    debug(f"Hook {hook} remains unchanged")
+                    debug(f"Created hook: {created_hook}")
+                else:


Suggested change

else:

gl_hook: dict = hook_in_gitlab.asdict() if hook_in_gitlab else {}

if "token" in hook_config:

debug(

f"The hook '{hook}' config includes a token. Diff between config vs gitlab cannot be confirmed"

)

gl_hook["token"] = ""

diffs = (

map(

lambda k: hook_config[k] != gl_hook[k],

hook_config.keys(),

)

if gl_hook

else iter(())

)

if any(diffs):

debug(

f"The hook '{hook}' config is different from what's in gitlab OR it contains a token"

)

debug(f"Updating hook '{hook}'")

updated_hook: Dict[str, Any] = project.hooks.update(hook_id, hook_config)

debug(f"Updated hook: {updated_hook}")

else:

debug(f"Hook '{hook}' remains unchanged")

This whole chunk instead of the rest of the code below the 'continue' after the creation of new hooks, to the end of the for loop. If you don't like the "" empty token assignement, you can leave that part as you had it, deal with the update of hooks with tokens, but then using the 'continue' to avoid another 'else' with further nested if's...

amimas · 2024-02-27T02:33:46Z

@TigreModerata - you helped with hooks processor recently on couple occasions. Wondering if you would be able to help with a review of this PR. Thanks!

Yes, sure, I'm sorry I was slow with my PR (very busy last week, unexpectedly). Good thing you did it anyway, because I wasn't going to add extra logic, rather I was just going to revert back to updating all hooks no matter what.

Hey 👋 . no worries at all. Wasn't expecting anything right away. I had a small opportunity and the idea about the extra logic. Thought I'd give it a shot.

Thanks for the feedbacks btw. I'm a python newbi 😛 If/when I have some time next, I'll look those. I didn't understand clearly about the latter suggestions. Looking at it from mobile. Will check it out later. Thanks again.

TigreModerata · 2024-02-27T12:23:11Z

Thanks for the feedbacks btw. I'm a python newbi 😛 If/when I have some time next, I'll look those. I didn't understand clearly about the latter suggestions. Looking at it from mobile. Will check it out later. Thanks again.

Yeah, you mentioned you were new to python, that's the only reason I even proposed any changes, the logic and everything was fine as it was, obviously :)
It's just that too many nested loops are frowned upon: they would slow things down considerably if you were dealing with larger sets of data, plus they make things hard to read (almost as bad a fault as being slow, in python ;) ). My suggestion is just one option for how to rewrite that chunk with just one level of nesting, to give you an idea, but you can probably find better ways.

amimas · 2024-02-28T03:51:50Z

Thanks for the feedbacks and suggestions. They all make sense to me. It is good to avoid nested conditions or loops in general; not just for python, I believe. I ended up moving the config diff logic to a separate function to make the hook processing logics a bit more readable.

When you get a chance, please take another look. Thanks again.

…ssertion

…-existent hook deletion

TigreModerata · 2024-02-29T12:53:13Z

@amimas last version looks great to me! :)

amimas · 2024-02-29T14:38:25Z

Thanks! Pushed one more small update based on the security tests being reported. It was complaining about the setting the token to an empty value, which makes sense. But, I realized I don't even need to do that. Please have a look if you get a chance.

TigreModerata · 2024-03-01T11:53:28Z

Thanks! Pushed one more small update based on the security tests being reported. It was complaining about the setting the token to an empty value, which makes sense. But, I realized I don't even need to do that. Please have a look if you get a chance.

OK, I had feared that the token setting to "" conflicting somewhere, plus it's really unnecessary (why introduce dodgy settings when they are not even needed?)...
Looks great now - I've said that before :D but still, can't find anything to object to :)

TigreModerata

tiny unimportant comment (if I were to nitpick anything "unpythonic"), just to show that one can write it in one line, but I don't think there is any real advantage

TigreModerata · 2024-03-01T11:58:57Z

gitlabform/processors/project/hooks_processor.py

+            else iter(())
+        )
+
+        if any(diffs):


Suggested change

if any(diffs):

return True if any(diffs) else False

It's not nitpicking. I'm glad you mentioned it. I do tend to forget about python style syntax. Thanks for pointing this out. It's small, simple, and easier to read instead of going through 4 lines.

… is needed

amimas · 2024-03-02T04:49:27Z

So... I was browsing through the code and realized there's already existing logic in the super class to check whether an entity requires update or not. We didn't need to write our own logic. I didn't know about it 🤦 .

Refactored the code and removed the diff checking logic from hooks processor. Used the existing logic instead.

TigreModerata · 2024-03-03T14:04:00Z

So... I was browsing through the code and realized there's already existing logic in the super class to check whether an entity requires update or not. We didn't need to write our own logic. I didn't know about it 🤦 .

Refactored the code and removed the diff checking logic from hooks processor. Used the existing logic instead.

gulp... I should've noticed! Well, on the bright side... it had to be done (the check added to the superclass), all the better that it has already :D
Apologising constantly is annoying, and fruitless, but do know I am slapping myself :D

amimas temporarily deployed to Integrate Pull Request February 25, 2024 04:57 — with GitHub Actions Inactive

amimas changed the title ~~Issue 685~~ fix: hooks processing fails when config includes token Feb 25, 2024

amimas marked this pull request as ready for review February 25, 2024 07:45

TigreModerata suggested changes Feb 26, 2024

View reviewed changes

amimas had a problem deploying to Integrate Pull Request February 28, 2024 03:48 — with GitHub Actions Failure

amimas added 10 commits February 27, 2024 22:52

test: minor formatting update and import ProjectHook

fe958bc

test: reproduce issue gitlabform#685

855b437

refactor: rename variables in hooks processor for clarity

6b16f47

fix: hook config with token should be updated successfully

b281ca1

test: add another hook with token in test cases

c3f3a40

test: add comments on test assertion for clarity and fix one of the a…

d1fdc70

…ssertion

chore: rename a variable and address mypy type checking issue

ad9c404

style: reformat by black

8ce2a8e

refactor: simplify logic to avoid nested if-else and add test for non…

3733e78

…-existent hook deletion

style: update formatting using black

3ab81d4

amimas force-pushed the issue-685 branch from 22dd86c to 3ab81d4 Compare February 28, 2024 03:52

amimas temporarily deployed to Integrate Pull Request February 28, 2024 03:52 — with GitHub Actions Inactive

amimas temporarily deployed to Integrate Pull Request February 28, 2024 12:43 — with GitHub Actions Inactive

refactor: no need for setting token to empty value

365a264

amimas temporarily deployed to Integrate Pull Request February 29, 2024 13:50 — with GitHub Actions Inactive

TigreModerata reviewed Mar 1, 2024

View reviewed changes

style: simplify if-else code using python style syntax

258d23e

amimas temporarily deployed to Integrate Pull Request March 2, 2024 04:21 — with GitHub Actions Inactive

refactor: use existing logic from super class to check if hook update…

f50a044

… is needed

amimas temporarily deployed to Integrate Pull Request March 2, 2024 04:46 — with GitHub Actions Inactive

amimas merged commit dd58a7a into gitlabform:main Mar 2, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: hooks processing fails when config includes token #693

fix: hooks processing fails when config includes token #693

amimas commented Feb 25, 2024 •

edited

codecov bot commented Feb 25, 2024 •

edited

amimas commented Feb 25, 2024

TigreModerata commented Feb 26, 2024

TigreModerata left a comment •

edited

TigreModerata left a comment

TigreModerata Feb 26, 2024

TigreModerata Feb 26, 2024

TigreModerata Feb 26, 2024

TigreModerata Feb 26, 2024 •

edited

amimas commented Feb 27, 2024

TigreModerata commented Feb 27, 2024

amimas commented Feb 28, 2024

TigreModerata commented Feb 29, 2024

amimas commented Feb 29, 2024

TigreModerata commented Mar 1, 2024

TigreModerata left a comment

TigreModerata Mar 1, 2024

amimas Mar 2, 2024

amimas commented Mar 2, 2024

TigreModerata commented Mar 3, 2024

	debug(f"Created hook: {created_hook}")
	debug(f"Created hook: {created_hook}")
	continue

-                else:
+               gl_hook: dict = hook_in_gitlab.asdict() if hook_in_gitlab else {}
+                if "token" in hook_config:
+                    debug(
+                        f"The hook '{hook}' config includes a token. Diff between config vs gitlab cannot be confirmed"
+                    )
+                    gl_hook["token"] = ""
+                diffs = (
+                    map(
+                        lambda k: hook_config[k] != gl_hook[k],
+                        hook_config.keys(),
+                    )
+                    if gl_hook
+                    else iter(())
+                )
+                if any(diffs):
+                    debug(
+                        f"The hook '{hook}' config is different from what's in gitlab OR it contains a token"
+                    )
+                    debug(f"Updating hook '{hook}'")
+                    updated_hook: Dict[str, Any] = project.hooks.update(hook_id, hook_config)
+                    debug(f"Updated hook: {updated_hook}")
+                else:
+                    debug(f"Hook '{hook}' remains unchanged")

fix: hooks processing fails when config includes token #693

fix: hooks processing fails when config includes token #693

Conversation

amimas commented Feb 25, 2024 • edited

codecov bot commented Feb 25, 2024 • edited

Codecov Report

amimas commented Feb 25, 2024

TigreModerata commented Feb 26, 2024

TigreModerata left a comment • edited

Choose a reason for hiding this comment

TigreModerata left a comment

Choose a reason for hiding this comment

TigreModerata Feb 26, 2024

Choose a reason for hiding this comment

TigreModerata Feb 26, 2024

Choose a reason for hiding this comment

TigreModerata Feb 26, 2024

Choose a reason for hiding this comment

TigreModerata Feb 26, 2024 • edited

Choose a reason for hiding this comment

amimas commented Feb 27, 2024

TigreModerata commented Feb 27, 2024

amimas commented Feb 28, 2024

TigreModerata commented Feb 29, 2024

amimas commented Feb 29, 2024

TigreModerata commented Mar 1, 2024

TigreModerata left a comment

Choose a reason for hiding this comment

TigreModerata Mar 1, 2024

Choose a reason for hiding this comment

amimas Mar 2, 2024

Choose a reason for hiding this comment

amimas commented Mar 2, 2024

TigreModerata commented Mar 3, 2024

amimas commented Feb 25, 2024 •

edited

codecov bot commented Feb 25, 2024 •

edited

TigreModerata left a comment •

edited

TigreModerata Feb 26, 2024 •

edited