Screwdriver: PR workflow to continue to subsequent jobs

[marian-r]

In the event of a commit the pipeline runs as normal and when the job A finishes then it continues to the ones which require job A to finish end so on.

However, in the event of PR, only the job with requires: [~pr] runs and it doesn't continue to subsequent jobs.

Example

We have the following screwdriver.yaml:

jobs:
  A:
    requires: [~pr, ~commit]
    steps:
      - echo A
  B:
    requires: [A]
    steps:
      - echo B

When a commit is made it triggers job A and when A finishes it continues to job B.
When a PR is made only job A runs and doesn't continue to job B.

I would expect that builds triggered by PRs follow the requires options.

If the PR and commit workflow are behaving in the same way, it is still possible to design the pipeline in the way that only 1 job is run on PRs, example:

jobs:
  A: @A
    requires: [~commit]
    steps:
      - echo A
  A-PR:
    <<: *A
    requires: [~pr]
  B:
    requires: [A]
    steps:
      - echo B

Opinions on this feature?

[drewfish]

+1 for the "PR can run multi-job workflow" feature. (not sure about the proposed syntax though.)

[petey]

I don't think there needs to be any syntax changes to support this. The workflow just needs to remove the arbitrary limits, and add $SD_PULL_REQUEST to subsequent jobs triggered by PRs.

[marian-r]

@petey that was exactly my point, just probably not explained clearly. My first code example shows workflow config without any changes - I would expect that to run both jobs on PR builds. The second code example is just to show syntax for achieving today's behavior with only one job to be run on PR build.

[yuichi10]

@marian-r @petey @drewfish
Hi guys!
We will progress this issue and I want you to check my proposal.
I think if subsequent jobs are triggered by PRs, it affects a lot to user and they need to fix their screwdriver.yaml. It will be so tough for user. So it will be better to require option like prChain.
When prChain is true subsequent jobs are triggered by PRs.
E.g.)

prChain: true
jobs:
  main:
    requires: [~pr ~commit]
  test:
    requires: [ main ]
  deploy:
    requires: [ test ]

At this time deploy and test job is work when main job is triggered by PR.

When implements this issue, I think there are three steps.

1. Allow PR to trigger subsequent jobs.
   Allow PR to trigger subsequent jobs when prChain: true is declared.
2. PR doesnot trigger subsequent jobs.
   When prChain are not declared ~pr job does not trigger subsequent jobs. So first we stop that first. This will fix this problem https://github.com/screwdriver-cd/screwdriver/issues/1082#issuecomment-429183057
3. Fix UI
   When PRs trigger subsequent jobs, pr have pipeline so we will fix UI.
   
   The difference between past is
   
   
   
   1. All PR events are shown in the gray area.
   
   
   2. Only latest PRs are shown in right side.
   
   
   3. When click PR-2:pr in the above image, the PR-2:pr event pipeline is shown in gray area.
   
   

[marian-r]

@yuichi10 thanks...I'm quite happy with the proposal 👍

[drewfish]

Yeah I agree a lot of folks rely on the existing behavior, so some kind of "opt in" to the proposed behavior makes sense to me too. The exact mechanism to opt-in kinda doesn't matter too much to me.

[tk3fftk]

@yuichi10 I have two questions:

1. Is ~pr that you mentioned in 1 using branch filtering feature?
2. Can prChain be set to only pipeline level?

[yuichi10]

@tk3fftk

1. Yes it is. The 1st step mean any ~pr and pr branch filtering feature does not trigger subsequent jobs.

1. Yes, prChain can be only set to pipeline level. To set prChain as job level makes pipeline complex.

[yuichi10]

Sorry I will change the three steps order
from

1. PR does not trigger subsequent jobs.
2. Fix UI
3. Allow PR to trigger subsequent jobs.

to

1. Allow PR to trigger subsequent jobs.
2. PR does not trigger subsequent jobs.
3. Fix UI

By this order, we can update api at each changes.

[tk3fftk]

• The PR-2:pr looks verbose. We should think about display name of ii. and a timing of status update.
• I think a pipeline triggers subsequent job unexpectedly, when prChain: true setting is added in PR with malicious intent. Should we handle it?

[yuichi10]

You're right we should think about display name. I think to show only PR-{pr-num} is enough, such as PR-2.
Every job, which include jobs triggered by PRs, shown in status check. Thus, the status update is conducted by each job.

When user set prChain: true and some problem happen, I think we do not need handle it.
How to use prChain: true is depend on users.
But of course we should keep improvements to make it easy to use prChain: true.

[tk3fftk]

I think to show only PR-{pr-num} is enough, such as PR-2.
Every job, which include jobs triggered by PRs, shown in status check. Thus, the status update is conducted by each job.

I agree these :+1:
If there are 3 jobs, 3 statuses are shown on PR page.

How to use prChain: true is depend on users.

I think so too, but I mentioned about usage of malicious intent. For example, there is a deploy pipeline for an application. A malicious user send a PR which breaks the application and adds prChain: true setting. Then Screwdriver triggers the deploy pipeline and the application will be broken.

[yuichi10]

If there are 3 jobs, 3 statuses are shown on PR page.

Yes it is.

I think so too, but I mentioned about usage of malicious intent. For example, there is a deploy pipeline for an application. A malicious user send a PR which breaks the application and adds prChain: true setting. Then Screwdriver triggers the deploy pipeline and the application will be broken.

I understand what you mean.

Even now we can deploy or publish from pr jobs, so I think we do not need to handle this problem.
For example SD admin should generate different token or something between pr and commit and deployed platform should check these token or something.
Also PR can not use secret without check of allow in PR secrets. So we can also protect secrets.

[jithine]

Fixing UI is a bit more involved, current side bar won't cut it. But this in inline with our designs for new pipeline page where events will also move to sidebar.

cc @DekusDenial

[yuichi10]

Then is it better not to fix UI?

sorry, the below fixing will be difficult, so if screwdriver have new pipeline page, it is better not to implement this below example.

Or show every PR job in sidebar?
e.g.)
PR-2:main
PR-2:test
PR-2:build
PR-5:main
PR-5:test
PR-5:build
Sort these PRs by PR number and pipeline order, I think it looks not bad.

[tkyi]

@yuichi10 Your proposal for PR UI might be an okay temporary fix while we wait for new UI designs/work to be done on PRs. Just depends how much time we think there will be between this feature getting done and the new UI design work to be done.

[yuichi10]

We think about UI and we have conclusion that we do not need fix UI.
I will present how the pipeline will be by stepwise.

The base screwdriver.yaml is below.

jobs:
  test:
    requires: [~pr, ~commit, ~pr:staging]
  deploy:
    requires: [test]

Now

• ~pr does not trigger
• ~pr:branch trigger jobs
• ~pr event is displayed in sidebar
• specific branch pr event is displayed in main view

Phase 1 (Allow PR to trigger subsequent jobs)

prChain: false

• same with now's pipeline
• ~pr does not trigger
• ~pr:branch trigger jobs
• ~pr event is displayed in sidebar
• specific branch pr event is displayed in main view

prChain: true

• every pr will chain
• every pr event is displayed in main view (not add PR-{pr-number} to pr jobs)

Phase 2 (PR doesnot trigger subsequent jobs)

prChain: false

• every pr does not trigger
• every pr is displayed int sidebar (add PR-{pr-number} to pr jobs)

prChain: true

• every pr will chain
• every pr event is displayed in main view (not add PR-{pr-number} to pr jobs)

We will implement these by adding PR-number when prChain is false and not adding PR-number when prChain is true.
I think to implement this view is not hard because we do not need to fix UI and these pipeline view is not bad for users.

[tk3fftk]

This is outdated

After discussion, we decided to separate this implementation into two passes.

First pass

Just adding prChain feature.

TODO

• [x] add prChain setting to data-schema
  
  
  
  • add prChain to Pipeline
  
  
  • https://github.com/screwdriver-cd/data-schema/pull/296
  
  
• [x] add prChain to config-parser
  
  
  
  • https://github.com/screwdriver-cd/config-parser/pull/81
  
  
• [x] models
  
  
  
  • pipeline
    
    
    
    
    
    • add statement using prChain on syncPR()
    
    
    
  
  
  • eventFactory
    
    
    
    
    
    • getJobsFromPR() passes prChain to workflow-parser
    
    
    
  
  
  • https://github.com/screwdriver-cd/models/pull/301
  
  
• [x] workflow-parser
  
  
  
  • fix not to add PR- prefix to a job name on getNextJobs() if prChain is true
  
  
  • https://github.com/screwdriver-cd/workflow-parser/pull/15
  
  
• [ ] enable to get type: pr events from pipeline page
• [ ] enable to show workflow graph

Second pass

Sync all relevant PR builds when PR closed/opened.
For example, when a pull request is closed, all triggered builds will be stopped.

TODO

[note] we tried to examine an implementation that adds job.prNums and build.prNum. But it's not completed yet.

• [ ] add prNums and prNum setting to data-schema
  
  
  
  • add prNums to Job (for check the job is triggered by PR or not)
  
  
  • add prNum to build
  
  
• [ ] models
  
  
  
  • handle isPR() and prNum() without PR- prefix
  
  
• [ ] launch
  
  
  
  • fix to use build.prNum instead of a job name
  
  
  • https://github.com/screwdriver-cd/launcher/pull/241 (WIP)
  
  
• [ ] api
  
  
  
  • filter with prNum on pullRequestClosed() and pullRequestSync()
  
  
  • add prNum to job.prNums when a pull request is opened
  
  
  • delete prNum from job.prNums and stop builds including prNum when a pull request is closed
  
  
  • do close and open tasks when a pull request synced
  
  
• [ ] scm modules
  
  
  
  • use a job name instead of PR- prefixed job name
  
  

[tk3fftk]

I edited our TODO list.

[sakka2]

In first pass, we've got review that it is better to create a new job when prChain flag is true.
https://github.com/screwdriver-cd/models/pull/301#issuecomment-444324846

So, we have changed the design about prChain to create a new job.

The design about prChain flag

Precondition:

• Only the prChain flag of main branch will affect the job chain.
• Even if prChain flag is changed in PRs, behavior of job chain does not change.
• This is because, it is quite dangerous that anyone can change prChain flag in PR and run subsequent jobs easily.

---

• I will explain what will be changed with prChain feature.

• There are 3 patterns.

1. normal commit
2. PR with prChain flag == false (prChain flag of main branch)
3. PR with prChain flag == true (prChain flag of main branch)

• Example screwdriver.yaml is below.

screwdriver.yaml

shared:
    image: node:8

jobs:
    first-job:
        requires: [~pr, ~commit]
        steps:
            - echo: echo first
    second-job:
        requires: [first-job]
        steps:
            - echo: echo second

1. ~commit

• At first, normal commit.
• Jobs table will be like below as normal.
• Nothing will be changed as before.
  

  Jobs table

second-job

UI (normal commit)

• UI will be like below as normal.
  

2. ~pr (#1) with prChain == false (prChain flag of main branch)

• Second, make PR#1 with no prChain flag in same pipeline.
• Jobs table will be like below. PR-1:first-job will be added. Just one job if prChain is false.
• Nothing will be changed as before too.
  

  Jobs table

second-job
PR-1:first-job   <-- newly added

UI (prChain == false)

• UI will be like below.
• Job PR-1:first-job are shown at side space.
  

3. ~pr (#2) with prChain == true (prChain flag of main branch)

• Third, make PR#2 with prChain flag (:true) in same pipeline.
• Jobs table will be like below. When prChain flag is true, all subsequent jobs in PR#2 will be added.
• Job name will be "PR-" + prNum + ":" + jobname.
  

  Jobs table

second-job
PR-1:first-job
PR-2:first-job     <-- newly added
PR-2:second-job    <-- newly added

• As shown above, all cases (~commit, ~pr#1, ~pr#2) make different jobs.
• So, it doesn't affect each other.

UI (prChain == true)

• UI will be like below.
• All jobs (commit, PRs) are shown in the event list, and nothing will be shown at side space. (It is better to show PR jobs in some suitable form at side space, but it will be the second pass or later)
  

[tkyi]

Your revised proposal looks really good!

Just wondering, if there's a PR#3 and there's multiple PR chains in one PR, what would the event commits look like?
For example:

prChain: true
shared:
    image: node:8

jobs:
    first-job:
        requires: [~pr, ~commit]
        steps:
            - echo: echo first
    second-job:
        requires: [~pr, first-job]
        steps:
            - echo: echo second
    third-job:
        requires: [second-job]
        steps:
            - echo: echo second

[d2lam]

@sakka2 Thanks for the new proposal. It looks good to me. I agree that we should interpret prChain from main branch only, otherwise it's dangerous.

For @tkyi 's case, I think it should just be 1 chain. PR-1:firstjob -> PR-1:secondjob -> PR-1:thirdjob What do you think? Whatever we go with, we need to document it clearly so that users don't get confused.

[jithine]

From job level it looks good and now has parity with original implementation. Also expecting SD_PULL_REQUEST will be set for all jobs in PR as per our documentation

Also for UI let's keep current way of separating Commit events vs Pull Request events. Refer updated mocks here https://github.com/screwdriver-cd/screwdriver/issues/1419

It should have no bearing on Pull requests tab whether prChain is true or false. Graph will reflect the state. cc @DekusDenial

[sakka2]

@tkyi @d2lam
Thanks for review!

Workflow will be the same as the present commit job like below.

jobs:
    first-job:
        requires: [~pr, ~commit]
        steps:
            - echo: echo first
    second-job:
        requires: [~commit, first-job]
        steps:
            - echo: echo second
    third-job:
        requires: [second-job]
        steps:
            - echo: echo second

So, this case in pr chain, PR-3:second-job will run 2 times.

PR-3:first-job --> PR-3:second-job  --> PR-3:third-job
                                             |
PR-3:second-job -----------------------------|

We think it should be 1 chain too, but we think this problem is the other issue.

[sakka2]

@jithin1987
Thanks for review!

OK, we will set SD_PULL_REQUEST for all jobs.
And this time, we will keep current way of separating Commit events vs Pull Request events.

[minz1027]

Got some issues from users with current implementation. There are a couple things to be fixed:

1. workflowParser not checking the prChain flag: have a PR open https://github.com/screwdriver-cd/workflow-parser/pull/17
2. prChain flag need to be pass in here later: https://github.com/screwdriver-cd/screwdriver/blob/master/plugins/builds/index.js#L278
3. subsequent PR jobs should inherit the prRef from its parent pr jobs otherwise checkout cmd will be wrong.
   https://github.com/screwdriver-cd/scm-github/blob/master/index.js#L420

[s-yoshika]

We are trying to implement UI for prChain and facing some problems.
These problems are based on API side (mainly event data).

Background:

To draw a workflowGraph for prChain, We have to use PR events, but some data aren't enough.

Problems:

1. PR events have no node ids in workflowGraph.
   
   
   
   • The workflowGraph of commit events has both name and id in nodes, but PR events has only node name.
   
   
   • The id of node is required to render graph.
     
     
     
     
     
     • https://github.com/screwdriver-cd/ui/blob/85edd7e3131ff94f8c836ecdcc3b3340372905f0/app/utils/graph-tools.js#L209
     
     
     
   
   

The workflowGraph in commit event:

curl -H 'xxxx' http://172.16.164.207:9001/v4/pipelines/1/events  | jq .[0].workflowGraph.nodes
[
  {
    "name": "~pr"
  },
  {
    "name": "~commit"
  },
  {
    "name": "first-job-2",
    "id": 1
  },
  {
    "name": "second-job",
    "id": 2
  },
  {
    "name": "third-job-2",
    "id": 3
  },
  {
    "name": "fourth-job-3",
    "id": 4
  }
]

in PR event:

$ curl -H 'xxxx' http://172.16.164.207:9001/v4/pipelines/1/events?type=pr  | jq .[0].workflowGraph.nodes
[
  {
    "name": "~pr"
  },
  {
    "name": "~commit"
  },
  {
    "name": "first-job-2"
  },
  {
    "name": "second-job"
  },
  {
    "name": "third-job-2"
  },
  {
    "name": "fourth-job-3"
  }
]

1. PR jobs have PR- prefix in their own names, but the workflowGraph in PR events have no prefix. The node name and edge name should have PR- prefix correctly in PR events.
   
   
   
   • Users are hard to distinguish which jobs are commit job or PR jobs.
   
   
   • Some codes can being dependent for job name. The below code searches a job id by the job name.
     
     
     
     
     
     • https://github.com/screwdriver-cd/ui/blob/ad8b31697ec9083d21af48eed63dfa2890eca377/app/components/pipeline-workflow/component.js#L23
     
     
     
   
   

We have to fix these problem in API.

[jithine]

@s-yoshika since we continue to keep PR events in separate tab, does the name prefix PR- matter much in terms of users understanding it ?

[s-yoshika]

@jithin1987 Yes, but API level, users can see different ids with same job name in workflowGraph of events.
And there can be some codes which dependent for job name.
https://github.com/screwdriver-cd/ui/blob/ad8b31697ec9083d21af48eed63dfa2890eca377/app/components/pipeline-workflow/component.js#L23

[d2lam]

@s-yoshika @sakka2 So we have this feature called restrictPR, and we are putting it under pipeline-level annotation. Do you think it makes sense to also put the prChain under the same level? For example:

annotations:
    screwdriver.cd/chainPR: true
    screwdriver.cd/restrictPR: fork
jobs:
   main:
       .... 

[sakka2]

@d2lam
Thanks for the proposal.
We agree to move prChain under the pipeline-level annotation.

[jithine]

@catto awaiting an example pipeline https://github.com/screwdriver-cd-test

[jithine]

@catto @sakka2 couple of issues.

1. Start pipeline from here should be taken out for chainPR it's starting an event under Events tab, not Pulls See the first event here https://cd.screwdriver.cd/pipelines/2539/events
2. For some reason second-job here keeps failing with a git checkout error https://cd.screwdriver.cd/pipelines/2539/pulls

So what's happening is subsequent jobs are entering a different path in setup scm step

[sakka2]

@jithin1987 Thanks for trying chain PR sample

1. We thought we had already fixed this problem. Does this PR have applied?
   https://github.com/screwdriver-cd/screwdriver/pull/1554

2. This problem happens in PRs from forked repository. sd-scm-github tries to git reset with the commit SHA of forked repository which dose not exist in original one.
   We are now fixing this problem not to use the SHA in all subsequent jobs.

[jithine]

@sakka2

1. I ran the pipeline today, it seems the referenced PR was merged 14 days ago. So something else is going on when restarting the job. Also allowing "Start pipeline from here" can lead to other problems since PR events are driven by Jobs and not by events itself. And here https://github.com/screwdriver-cd/ui/blob/master/app/pipeline/pulls/route.js#L36 we are restricting PR event for a PR number to 1
2. 👍 for fixing it.

[sakka2]

@jithin1987

So something else is going on when restarting the job.

In my local environment, this problem does not happen. Could you tell me the API version? I will check the difference in source level.

Also allowing "Start pipeline from here" can lead to other problems since PR events are driven by Jobs and not by events itself.

Could you explain more detail about this problem? I think showing only the latest event is OK.

[sakka2]

TODO

We will show the rest tasks of chain PR feature below

• [ ] Fix the error from forked repository
  => Now, job error happens in PRs from forked repository, because sd-scm-github step tries to git reset with the commit SHA of forked repository which dose not exist in original one.

• [ ] Sync all the PR jobs when chainPR flag of the pipeline branch is changed
  => Now, even if chainPR flag of the pipeline branch is changed, already existing PR jobs are not synced.

• [ ] Make functional test of chain PR feature

[jithine]

I noticed that on cd.screwdriver.cd in the test repository for pr chain

On Tue, Apr 2, 2019 at 1:31 AM Teppei Minegishi [e-mail]
wrote:

@jithin1987 https://github.com/jithin1987

So something else is going on when restarting the job.

In my local environment, this problem does not happen. Could you tell me
the API version? I will check the difference in source level.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/screwdriver-cd/screwdriver/issues/1010#issuecomment-478897940,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AALyZjLDrjsmgNfgGVOQd6O_VSTY6eEbks5vcxV_gaJpZM4TTID7
.

>

Sent from Gmail Mobile

[jithine]

@sakka2

In my local environment, this problem does not happen. Could you tell me the API version? I will check the difference in source level.

I am a bit unsure on the behavior. If you see the first event here https://cd.screwdriver.cd/pipelines/2539/events I recall I got that after restarting second-job which exists on PR.

But now I am not able to reproduce it, I did restart on second-job on PR-9 it came correctly in pull request section itself. However it now only shows the second-job for the PR-9. It should probably aggregate so that all job's builds are visible.

https://cd.screwdriver.cd/pipelines/2539/pulls

[sakka2]

@jithin1987

But now I am not able to reproduce it

We will test again especially this point, and will make a functional test to guarantee the behavior.
Pease let me know if the problem happens again.

It should probably aggregate so that all job's builds are visible.

Oh that's true. It is a matter can't move to the detail page. We will fix this.

Do you think we need Start pipeline from here feature?

[jithine]

@sakka2

Do you think we need Start pipeline from here feature?

To make it simpler we can remove Start pipeline from here for pull requests for now and introduce it back later.

[sakka2]

@jithin1987
Thanks! we will consider this.