Ciac 9706

[edik24]

Fixing CIAC-9706

In the current flow when we check if tim reviewer is needed - we use graph functions for example:
BaseContent.from_path(CONTENT_PATH / file)

When a new pack contributed, the graph doesn't find it and throws an error, that stops the handle external pr.
I've added a check in the is_tim_content that if the pack is new (not found in master), I'll checkout the branch and check in the branch.
If the branch checkout is not successful (sometimes it's private and we don't have permissions to checkout) - the script won't ask review for tim (and if needed, should be asked manually).

Status

• In Progress
• Ready
• In Hold - (Reason for hold)

Related Issues

fixes: link to the issue

Description

Fixing CIAC-9706 in the handle external PR flow

[ShahafBenYakir]

Added missing comment

[ShahafBenYakir]

Good job.
See my comments

[ShahafBenYakir]

What exceptions this is intended to catch?

[edik24]

the BaseContent.from_path(CONTENT_PATH / file) sometimes returns "not found" for example for images or for items that are not formatted correctly (like incident_field or incident_type missing a mandatory key)

when not found - content_object.in_pack returns an error.
This is what I want to catch.

[ShahafBenYakir]

Why would this happen?

[edik24]

This will happen if we don't have permissions to checkout remote branch. this happens sometimes.
depends on the remote settings, now all of them allow to checkout their branch

[ShahafBenYakir]

Why aren't we always checking out the contributor repo? it is more accurate...
Also, you are calling check_new_pack_metadata for every file, this should not be the case it should only be called once...

[ShahafBenYakir]

Basically the logic of the check_new_pack_metadata function should be the logic of the is_tim_content

[edik24]

is_tim_content is only doing the:
BaseContent.from_path(CONTENT_PATH / file)
and based on the results it chooses whether to go to check_new_pack_metadata - which will checkout the remote branch if this is a new pack and not part of the master.

or if this is in the master and the from_path returned an item - I will go to check_if_item_is_tim

you're correct about running for each file, I've added break after.
about checking out every time - I think it saves time that if we don't need to checkout (if this is not a new pack - since most of the PR's are not new) then no need to go there.

[ShahafBenYakir]

unless we have a really good reason why a checkout wouldn't work in some cases I would like to remove this.

[edik24]

As I noted before - there are some cases (even now) when the checkout is not successful.
At first I just returned false - so if the checkout wasn't successful, we just not assign TIM reviewer.
But then I thought that if i'm already working on this area, I should cover all options - so I added that.

So the logic:
if in the pack exists in master - check the items from graph
if the pack new - try to checkout and then check items from graph
If can't checkout - check items manually

in the last days checkout failed for those PR's (checkout is also done to check support level):
#34859 - https://github.com/demisto/content/actions/runs/9522341776/job/26251687499
#34827 - https://github.com/demisto/content/actions/runs/9484881582/job/26135465734
#34769 - https://github.com/demisto/content/actions/runs/9403458275/job/25900055331

so it is happening from time to time

[ShahafBenYakir]

can you explain this if?

[edik24]

Yes,
when we call the checkout - it first creates a new forked_remote_branch name in line 117:
forked_remote_name = f'{fork_owner}{repo_name}{branch_to_checkout}_remote'

so if there was already a try for checkout, for example when checking the support level, the next try to checkout will fail and return an error.

In this case the except clause will ignore the "already exists" error and would continue to line 139 to checkout.

[ShahafBenYakir]

add this as a comment to the code

[edik24]

add finally instead of 2 falses