feat: enhance branch matching with regex

[BenjaminDecreusefond]

Hi @alfespa17 !

Following our discussion on this topic I'd like to propose a new way to do the matching for the branch list. The idea is as follow

• Be able to say I want to match all branch except these ones
• Keep the previous logic with startsWith branch

The only thing is that I do not automatically return True when the branch is the same as the workspace's default branch. I don't know if this is an issue ?

It's just a proposal for us to be able to achieve our goals ! Lemme know if you approve it :)

Regards

[BenjaminDecreusefond]

hey @alfespa17 would you know why in the previous behavior we were returning true if the branch matched the workspace default branch ?

[alfespa17]

hey @alfespa17 would you know why in the previous behavior we were returning true if the branch matched the workspace default branch ?

I the beginning we only supported push events in one single branch the default one, but that behavior changed some time ago

[BenjaminDecreusefond]

Okay ! So do you think we can merge that PR ?

[alfespa17]

Okay ! So do you think we can merge that PR ?

I will check the logic later

[stanleyz]

The only thing is that I do not automatically return True when the branch is the same as the workspace's default branch. I don't know if this is an issue ?

It's not strictly required, but I believe that's the same thinking behind the below codes.

1. Return true if it matches the workspace default folder
   

   terrakube/api/src/main/java/org/terrakube/api/plugin/vcs/WebhookService.java

   Line 224 in e30c986

   if (file.startsWith(workspaceFolder)) {

2. Use the workspace default template if the branch is the workspace default branch.
   

   terrakube/api/src/main/java/org/terrakube/api/plugin/vcs/WebhookService.java

   Line 98 in e30c986

   if (webhookResult.getBranch().equals(workspace.getBranch()) || templateId == null || templateId.isEmpty()) {

If you are going to change the behaviour here, it's probably good to make it consistent for the above item 1?

This PR greatly enhances the flexibility to match different branches, but since you've already gone down the path of some regex matching, should we use a similar behaviour as the file matching? Or make them consistent?

terrakube/api/src/main/java/org/terrakube/api/plugin/vcs/WebhookService.java

Line 229 in e30c986

if (file.matches(triggeredPath[i])) {

[BenjaminDecreusefond]

hey @stanleyz !

I think you're right ! It seems like the code is already matching with regex folder paths ! Maybe we should just remove the behavior of returning true if it matches workspace's default files ? To me it seems like enough

[BenjaminDecreusefond]

hey @stanleyz @alfespa17 !

I just changed the behavior to use regex for branches as well so everyone is using the same logic. I also remove the workspace default path matching for the file finding so we have much more freedoms when designing webhooks !

thanks !

[alfespa17]

I added some comments @BenjaminDecreusefond, I think you only need to leave this part webhookBranch.matches(branch)

[BenjaminDecreusefond]

Hi @alfespa17 !

I'm not seeing your comments unfortunately ! :/
However, I think keeping the behavior of matching the workspace default branch will create duplicate trigger:

• One trigger from the base webhook of the default workspace branch
• Second trigger from the webhook created for feat or fix branches

What do you think ?

[alfespa17]

These are the comments

[alfespa17]

Hi @alfespa17 !

I'm not seeing your comments unfortunately ! :/ However, I think keeping the behavior of matching the workspace default branch will create duplicate trigger:

• One trigger from the base webhook of the default workspace branch
• Second trigger from the webhook created for feat or fix branches

What do you think ?

You can do this with the existing code, the default branch will trigger the default template and in the webhook branches you can put "feat,fix" and that should work.

[BenjaminDecreusefond]

thanks @alfespa17 !

Hmmm I think since there is already a webhook created by the workspace by default which matches its default branch master and then another webhook which matches for instance feat branch, removing this behavior

if (webhookBranch.equals(webhook.getWorkspace().getBranch())) {
            return true;
        }

will not break the current logic ?

[alfespa17]

thanks @alfespa17 !

Hmmm I think since there is already a webhook created by the workspace by default which matches its default branch master and then another webhook which matches for instance feat branch, removing this behavior will not break the current logic ?

If you remove this part changes in any directory will trigger a job inside Terrakube, for example if we have 10 folders managed inside Terrakube we only want to trigger the workspace execution for changes inside 1 particular folder.

            if (file.startsWith(workspaceFolder)) {
                log.info("Changed file {} in set workspace path {}", file, workspaceFolder);
                return true;
            }

But if you have like several terraform modules and your workspace depends on that particular module you can also have changes trigger a job for some expression like this "modules/my-dependency-module/*"

this is why you see this in the current code:

for (String file : files) {
            if (file.startsWith(workspaceFolder)) {
                log.info("Changed file {} in set workspace path {}", file, workspaceFolder);
                return true;
            }
            for (int i = 0; i < triggeredPath.length; i++) {
                if (file.matches(triggeredPath[i])) {
                    log.info("Changed file {} matches set trigger pattern {}", file, triggeredPath[i]);
                    return true;
                }
            }
        }

The issue behind that logic was this one

#654 (comment)

[BenjaminDecreusefond]

What I don't understand from what you're saying is that how can it trigger a job in terrakube whatever the directory we edit since we use a regex matching just after in the code ? Maybe i'm silly and I'm missing smth ? 🤔

[BenjaminDecreusefond]

@alfespa17 I updated the PR !