Need to refactor regarding build triggers

[yakanechi]

What happened: The trigger processing has become more complex, and different trigger processes are intertwined in the code, making it very expensive to identify and fix problems that occur around triggers. Also, when a bug related to a trigger is fixed, a new bug is likely to occur. (The latest example is #2980.)

These problems are especially likely to occur in complex conditions such as scenarios that combine external triggers and restarts. However, since both the restart process and the external trigger process are written together with the normal trigger process, it is difficult to perform stand-alone tests.

What you expected to happen: The code should be highly readable and easy to maintain. In addition, it is desirable to have completely separate flows for each trigger. Especially this file: plugins/builds/index.js

How to reproduce it:

[yakanechi]

LY team is considering refactoring the plugins/builds/index.js file over the coming weeks. The following is what we are considering at this time

1. Creating a triggers directory in the plugins/builds directory and consolidating all trigger processing in that directory.
2. Split the processes such as And, OR, Remote Join, Remote trigger, etc. into separate files for each function.
3. Split a huge function into several smaller functions.
4. Simplify complex functions to the extent possible (Shallow nesting, etc.)
5. Add tests for changed/added functions.

The idea is represented in the following rough image. We want to split the files by trigger feature as follows, which were integrated in index.js. This is because we can concentrate our work only on the triggers we are interested in.

The purpose of this work is to enhance the testing of triggers so that potential bugs can be identified early and fixed more quickly. As a first step, we are separating the functionality of each trigger to make it easier to test per triggers.

[yakanechi]

@tkyi @yakanechi Is there any possibility of conflict with the work being performed by pipeline stages (#2767, #2870, etc...)? Especially since the changes in #2767 are completely duplicative, there is a very high likelihood of conflicts when this work is done. What are some things we should be aware of or any comments on the task and timing of the work? Is it better to wait until the pipeline stages implementation is finished? Thank you.

[y-oksaku]

I have found some behaviors that are contrary to expectations.

Case1 (Fixed)

In the pipeline described below, the target job is not triggered when only the a build succeeds. The condition requires: [ ~a, a, b ] is equivalent to requires: [ ~a ], which implies that the target job should be triggered by a.

jobs:
  a:
    requires: [ ~commit ]
  b:
    requires: [ ~commit ]
  target:
    requires: [ ~a, a, b ]

Case2

In the pipeline described below, the target job is triggered twice when both a and b succeed. It is expected to trigger only once. However, the current behavior might be preferable if we utilize distinct metadata in a and b.

# Upstream(1)
jobs:
  a:
    requires: [ ~commit ]
  b:
    requires: [ ~commit ]

# Downstream(2)
jobs:
  target:
    requires: [ ~sd@1:a, ~sd@1:b ]

[ibu1224]

We have added tests in preparation for refactoring, and the setup is complete.

For the refactoring, I have proceeded with file splitting by each trigger. This is done to enhance the readability and maintainability of the code.

Please refer to the following branch for details: https://github.com/screwdriver-cd/screwdriver/compare/master...sonic-screwdriver-cd:screwdriver:refactoring-triggers

All unit tests succeeded, and no ESLint errors were detected. Functions used commonly across triggers have been consolidated into helpers.js. This consolidation will facilitate the creation of future tests. The code still has areas of redundancy and cases where error messages are not appropriate, as it is just a file split at this point.

What to do next

Moving forward, the process will proceed as follows:

1. Refactoring of OR triggers
2. Refactoring of AND triggers
3. Refactoring of RemoteTrigger
4. Refactoring of RemoteJoin
5. (optional) Addition of tests for each function in helpers.js
6. (optional) Refactoring each function in helpers.js

What should I watch out for in refactoring?

During refactoring, changes will be made on a different branch from the main branch.

Conduct a review for each trigger. Reviewers and implementers should be aware of the following

1. Regarding Nested if Statements: Avoid nesting if statements more than two levels deep. Utilize early returns and AND/OR condition chaining.

2. Regarding Nested for-loops: Endeavor not to use if statements within for-loops; instead, exclude non-conforming items from arrays beforehand with filters etc. As the continue statement is prohibited by ESLint settings, do not use it.

3. Regarding Promises and async/await: Use async/await and employ try-catch for error handling.

4. Regarding Object Types (JSON Format): After initializing an object, avoid adding properties later on. This prevents difficulty in tracking properties and complicating state tracking.

5. Regarding Arguments of Object Type: As much as possible, avoid using object type arguments (keyword argument format). However, if arguments are provided externally (e.g., functions called from server.expose), their use can be inevitable.

6. Regarding Object Type Keys: Do not abbreviate keys (explicitly state the keys). This ensures that changes in variable names do not inadvertently change behavior.

7. Regarding let/const: Use const wherever possible to prevent reassignment of variables. Avoid using let and consider using immediate functions to create a new scope where necessary.

8. Regarding Variable/Function Names: Use variable/function names that accurately indicate intentions without needless abbreviations. For object type variables, append suffixes such as Object or Map to avoid confusion with arrays.

9. Regarding Unary Plus Operator: Do not use it to convert strings to numbers. Prefer to use a dedicated function capable of handling errors for such conversions to mitigate bug risks.

10. Regarding Classes: Create classes for each trigger to manage frequently-used but unchanging elements (like JobFactory) within properties and methods. As the build-relevant calls are repeated upon each build completion, managing the current pipeline, event, job, and build within classes improves efficiency over passing them as function arguments each time.

[yk634]

I have found a problem with the trigger on restart.

What happened:

In the following workflow, after all jobs have succeeded, the hub job is restarted.

jobs:
  hub:
    requires: [ ~commit, ~pr ]
  a:
    requires: [ ~hub ]
  b:
    requires: [ ~hub ]
  c:
    requires: [ b ]
  target:
    requires: [ a, c ]

If job a is completed, job b is running, and job c has not been created, the target job will be triggered.

Cause:

In the current behavior, the completion condition for a join build at restart is that the latest build with the same groupEventId has succeeded. If a new event has not created a build for a job that should wait, we look at the build status of past events to determine that it is complete.

How to Fix:

First, I think we need to discuss the specifications for join builds at restart.

My thoughts are as follows:

It is necessary to determine if the build of the job that should be executed in that event is complete. To do this, we need to get a list of jobs that should be executed at that event. I believe this can be accomplished by recursively searching event.workflowGraph.edges. (Not easy if we take into account even remote pipelines, though.)

And I think it's a good idea to make sure those jobs are completed in that event.