App approval logic integration with github

[pnispero]

App approval logic integration with github

Functionality wise, we want any deployments to production to be code reviewed and approved

This description is tentative, still thinking about what exactly were going to implement, going to meet with Claudio, Jerry

1. Outline the API calls we want from core-build-system backend

• (GET) check if this repo/tag has been code reviewed
• (POST) after deployment if successfully ran and tested, then call backend to merge pull request
• etc.

2. Add the logic in the CLI/playbooks to use those API's

• When user chooses to deploy to production, call to backend api to check if the tag has been code reviewed

3. core-build-system backend monitor repo for any pull requests, and if merged then start the build, test workflow?

Use cases:

1. if a pull request is CREATED, then start a build/test, report back to the comments if successful
2. if a reviewer requested changes, developer updates the pull request, and trigger the build/test again, report back to comments
3. if a pull request is approved, merge onto main and trigger the build/test again

[pnispero]

Update: Meeting with Claudio, Jerry about app approval workflow,
Notes here:
https://confluence.slac.stanford.edu/display/LCLSControls/CD+-+App+approval+functionality
https://docs.google.com/presentation/d/12AaA8-oZA2TPj8ieMkwjSldTLGwtfP1mNMYR335nFpc/edit?usp=sharing

Workflow we proposed:

Claudio workflow response:

We are on the same page, and this is a good starting point

[pnispero]

Other todo:

• sending build results to artifact storage but only on a pre-merge branch
• Utilize existing env variable ADBS_BUILD_TYPE, but if the build is triggered by the backend (meaning a PR is made or a premerge branch is made, put in 'tagged' for ADBS_BUILD_TYPE?)

[pnispero]

Update

1. Look at how to integrate the approvers (lists and groups)
2. list is the list of individual approvers
3. groups is a group of approvers (probably a unix group)
4. approval methodology/requirements (unanimous, at least one/two approval, key individual(s)) - this should be a field in the component db
5. testing criteria (all, percentage, something else)
6. approval override.
7. override notifications until override was formally reviewed.

[pnispero]

Update - Approval logic

Good news, github natively already has features to require PR reviews. For every repo under settings > Branches > Branch Protection rule > add classic rule.

1. There is an option for Require a pull request before merging. Check that box, then you get subrules, with one important one Require approvals, and you can specify how many approvals are needed. And/or Require review from Code Owners. Code owners is a specific file you put in .github where you can specify the github usernames of the "owners" of the code, you can do certain files, or the whole repo itself. Also possible to specify github teams as the "owners". More details on CODEOWNERS.
   

2. There is also an option of Allow force pushes, which can serve as our 'override/backdoor' to the reviews.
   

Plan

1. A repo owner follows the 2 steps above for the initial configuration, then the backend can handle the rest.
2. Since most of the functionality we are looking for is already there (requiring approval from approval people which criteria specified in CODEOWNERS, and a backdoor for override)
3. There are 2 missing features:

   1. notifying reviewers after a force push has been made (Luckily our webhook can be notified when a force push occurs and the backend can handle the rest).
      

   2. not allowing merge until test criteria is met. (One way to solve the test criteria is to make the corebuildsystembackend as one of the CODEOWNERS required for approval, and the backend can approve the PR once it sees the tests criteria is met) Although it seems like its not allowed yet https://github.com/orgs/community/discussions/23064. I think we shouldnt worry about the tests criteria being met automatically by backend just in case like others have mentioned it may not be consistent if other systems are failing but not the repo in review. So really it should be up to the approvers to look at the test results the backend commented, and see if they want to approve it.

      1. If we still want the test criteria to be enforced there is an ALTERNATIVE: There is a setting Require status checks to pass before merging and Do not allow bypassing the above settings and Restrict who can push to matching branches, and in here we can specify the backend app. What this does is only allow the backend to merge the branch (if the status checks pass), and noone else can bypass. In this way we can have the backend after test criteria is met then can merge. If we are worried about override, we can make a CLI command to override the branch approval to the backend to then merge. Or you can add one more member maybe the repo owner who can override in case of emergency if the backend is down. For the status checks, there is an api endpoint the backend can set the status to any string like "Success" to a specific commit. And the specific commit is the latest commit in the PR, which is given in the pr webhook notification. We can use this when tests meet criteria, then set status, and merge (or pre-merge). api for status github app .api for status non-app. The non-app api is the one i tested with

[pnispero]

Update

we'll use this, but just for approvers. And we want an ADMIN CLI to be able to poll the branch protection rules and CODEOWNERS and make sure they are set correctly. Don't worry about test criteria for now, we'll rely on reviewers to check the test results.
ACTUALLY, the only thing for test criteria we can do is add a commit status check after the automated build. So thats the only logic we need to add in the backend. No extra database fields needed.

1. Add a summary check status for automated build/test commit that says "Passed 15 of 20" and for failing "Failed 10 of 20"

[pnispero]

We should add a field for the component database like testFormat, and here we can specify the testing format like JUnit, or TAP, or JSON, or nothing and then can have the approvers look review the testing themselves.

Write somewhere else appropriate:

For deployment/testing on production

1. Have a status check for pamm production testing - Which is essentially, block off the merge to main, until an approver says production testing is finished. Workflow can look like this:
   1. User creates branch with bs create branch --production-tests-required
   2. the flag productionTestsRequired would be set to true in the db for the component
   3. code is approved by CODEOWNERS, and build succeeded.
   4. Unable to merge because there is one more status check the backend created Production tests unfinished.
   5. Then once production tests are done, user will use bs mark --production-tests-passed or --production-tests-failed
   6. User is ready to merge PR to main
2. And the gatekeeping logic for deployment to only occur on pamm.

[pnispero]

patrick - want to figure out if we can combine the multiple os'es into one comment and status
take claudio's comment in slac and add it to some documentation somewhere
solved - can add a field to build variables commit_sha, and then can check when any build finishes, check for other builds in same pr/commit_sha. then if match then concatenate comment together. But how if both threads see they are together? They would have to send each other data? or make one of them store the data in the database as a new componentBranchBuildLog. then have another thread come in and check the log to add the comment/status. The only issue here is the dependceny of the uniqueness of the commit_sha, sure the commit is unique, but a build doesnt just start only on new commits, when a PR reopens it starts a build and that can have the same commit as the previous automated build - But actually if we delete builds from componentBranchBuild then the old builds on the same commits wouldn't be there, so maybe this is not a problem.

Patrick - summarize this and propose to Claudio if a good design
Proposal:
In an automated build, there can be multiple going at once (one for each operating system), and there is a new comment for each build since they are operating on different threads. We want a way to combine each build report into just one comment (and one commit status update (new feature adding)).

1. So, I was thinking we can add a field ADBS_COMMIT_SHA to componentBranchBuild -> buildCustomVariables.
2. Then in the processBuild(), when an automated build is finished, it will query the componentBranchBuild database for any other builds that have the same ADBS_COMMIT_SHA and pr_id. If so combine the log data, then submit a new entry in componentBranchBuildLog (this would be a new collection), and the entry will contain the log data.
3. Then we would have another thread that parses the componentBranchBuildLog, and will add the comment accordingly, then mark as done but will leave the entry there for logging purposes.
4. This would also require removing entries from componentBranchBuild once they are complete, otherwise they can interfere with new builds that possibly may have the same ADBS_COMMIT_SHA and pr_id.

[pnispero]

Figured out how to solve the above problem. We can have multiple status checks for each commit. So instead of combining each os build into one status check, we can do one for each. And you wont be able to merge until each status check is passing.

Pending build

Build success