Flag for manual review design doc #6138
Conversation
More revisions will follow
| 3. Each additional round of changes will be reviewed within one business day. | ||
| 4. When all review comments have been resolved, the PR can be merged into the master branch for deployment. | ||
| 2. If your PR triggers any [additional automated checks](./manual-review-triggers), a bot will leave a comment and request a manual review from the **frontend-review-group** | ||
| - Wait for the review group to provide a manual review - it should be completed within one business day. |
There was a problem hiding this comment.
| - Wait for the review group to provide a manual review - it should be completed within one business day. | |
| - the frontend-review-group completes reviews within one business day |
can we change the state of the PR to make the frontend-review-group a required reviewer?
There was a problem hiding this comment.
Unfortunately the API doesn't let us do that. All we can do is say who we want to request a review from.
However, I included a section talking about other ways that we could enforce that they wait for a manual review.
| There are situations where it makes sense to disable certain rules, but those will have | ||
| to be evaluated on a case-by-case basis. | ||
|
|
||
| ### Adding icons |
Co-Authored-By: Rian Fowler <rian.fowler@adhocteam.us>
Co-Authored-By: Rian Fowler <rian.fowler@adhocteam.us>
| 1. Getting approval from a member of the VSP *frontend-review-group* | ||
|
|
||
| The final approval from the review group is often not helpful, and it slows the process down. | ||
| This frustration often leads to VFS teams ignoring this process and they will directly ask for a VSP review before their PR is ready. |
There was a problem hiding this comment.
"Background" should include some details about the CODEOWNERS changes, and how that means that VSP is withdrawing from the code review process.
without explaining that, the "flag for manual review" stuff isn't really justified / doesn't connect to the background.
There was a problem hiding this comment.
I avoided that because that felt like part of the solution, and the template said to keep that out of the background. I'll try to phrase it in a way that keeps the emphasis on the problem and not the solution.
|
|
||
| ### Debugging | ||
|
|
||
| If a user sets up the proper environment variables locally, the script can be run in a local environment and the logging output can be viewed. Otherwise we will rely on viewing CircleCI logs. |
There was a problem hiding this comment.
any details on environment variables? doesn't have to be an explicit list in this doc, but maybe a link to documentation about them? or a link to code showing what environment variables are expected?
There was a problem hiding this comment.
I put the environment variables in the same doc with the list of triggers and linked to it. I'm not convinced that that is the best place, so that may change as I clean things up.
|
|
||
| ## Specifics | ||
|
|
||
| ### Detailed Design |
There was a problem hiding this comment.
the idea with the "detailed design" is that it should be sufficiently explained to hand off to a junior engineer who's just joining and say "go build this", and they should generally be able to figure out what needs to be done from it without any big open questions.
that framing is partly a way to put down more detailed documentation, but also partly so that other reviewers can review those decisions in detail and speak to them. in this doc, the content is very high-level, so there isn't really space for reviewers to evaluate more technical details.
a few things that immediately stand out to me as underspecified in that framing:
what's "the GitHub API"?
- what API methods are called?
- how is it depended on --- direct REST calls? through a library? does CircleCI provide this?
- how are we authorizing to the GitHub API?
how do we check out the current PR branch vs. master? is that something CircleCI provides? does CircleCI have docs on this?
- what kinds of things warrant manual review? do you have examples of the kinds of things we'd want to check for (whether or not they're in our first pass)?
Co-Authored-By: Andrew Gunsch <andrew.gunsch@adhocteam.us>
Pulled from PR description in department-of-veterans-affairs/vets-website#11670
Co-Authored-By: Andrew Gunsch <andrew.gunsch@adhocteam.us>
That seems to be just for the template
There was a problem hiding this comment.
Design doc location: maybe back in the design-docs folder, and prepended with a date? we don't really have a convention yet for where these live, but the engineering directory is pretty convoluted already with a bunch of different files.
Couple small comments here, but otherwise this looks pretty good! adding my approval (but make sure to get others' also before submitting).
| @@ -76,7 +94,11 @@ No other context is included. | |||
|
|
|||
| ### Security Concerns | |||
|
|
|||
| We should make sure that the Github bot auth token is kept secure and it should not be associated with an individual's account. The sole communicators in the system are CircleCI and Github. This isn't really a security concern, but more of a reliability one: if one of those services goes down we do not have a fallback plan. | |||
| We should make sure that the Github bot auth token is kept secure and it should not be associated with an individual's account. | |||
| The current strategy for storing this token is as an [environment variable in CircleCI](https://ui.circleci.com/settings/project/github/department-of-veterans-affairs/vets-website/environment-variables) called `GITHUB_TOKEN`. | |||
There was a problem hiding this comment.
From the CircleCI docs:
Note: secret masking will only prevent the value of the environment variable from appearing in your build output. The value of the environment variable is still accessible to users debugging builds with SSH.
who has SSH access to these builds?
There was a problem hiding this comment.
My understanding is that anyone who has an SSH public key on Github will be able to connect. I re-ran a job with SSH and this is what I see:
So anytime we deliberately kick off a job with SSH access, we should be able to see whoever connects to it.
There was a problem hiding this comment.
anyone with an SSH public key? So, that sounds to me like the GitHub bot auth token can be accessed by anyone?
There was a problem hiding this comment.
I found a more specific answer in the docs:
In order to SSH into a CircleCI build, the username must be one which has access to the project being built!
When I try to go to the project website in a private window it blocks me and forces me to authenticate with Github or Bitbucket.
I created a fake github account to test this and was able to login and view the project in Circle, but I can't even look at the project settings. I get errors when I try to follow the project, and I can't see the list of branches either. The option to trigger an SSH build is disabled as well:
After seeing all of this, I think that project access in circle requires an account to be a part of the Github organization that the repo is associated with. I've looked around at various settings to try to get a more definitive answer, but I'm not an organization admin in Circle so there are certain things I can't see. I wonder if this page would be helpful.
There was a problem hiding this comment.
https://discuss.circleci.com/t/disable-ssh/31057 has a couple key things:
Any user with write capabilities will also have SSH access. [...] any user with write permissions could theoretically also expose secrets via commands issues through the config.yml file.
Related question to investigate: how often do we hand out write permission on vets-website/vets-api, or do we generally keep people's access to read?
A good first step is to ensure your deploy keys only have read access to your repos.
This made me wonder, what kinds of access does our bot token have? Can we keep that as minimal as possible? (read-only + posting comments?).
Other possible idea: it sounds like contexts might be useful here.
There was a problem hiding this comment.
Do you know who gives write access to people?
Our bot token has the public_repo scope. I believe that this is the least permissions that we can give it in order for it to do its job, but it does include write access for our public repos.
Limits access to public repositories. That includes read/write access to code, commit statuses, repository projects, collaborators, and deployment statuses for public repositories and organizations.
Here is a relevant Slack thread.
Contexts are something that I haven't explored deeply. I'm interested to see how we could use them, but I don't have permissions to manage security groups so I'm a bit limited.
There was a problem hiding this comment.
Do you know who gives
writeaccess to people?
VA has an entire GitHub admin group. you can also talk to @ricetj about longer-term strategy for ownership on these repos, or (more immediately helpful) see if you can track down an admin for vets-website and vets-api to see how we've handed out write access so far. it's possible that most people just have read access and that this isn't a concern, but write access is typically needed for managing issues, which might mean we've handed it out more broadly.
Our bot token has the
public_reposcope. I believe that this is the least permissions that we can give it in order for it to do its job, but it does include write access for our public repos.
hmm, this set of information is making me more and more concerned 😬
what does the bot need from public_repo that it can't get from (no scope)?
There was a problem hiding this comment.
FWIW- Write access is needed by a GitHub team for codeowners to trigger so every VFS member has write access to the repo.
All of the FE group on VSP has admin access.
Generally everyone has write access.
Co-Authored-By: Andrew Gunsch <andrew.gunsch@adhocteam.us>
Pulling this out from the PR with the design doc (#6138 ) so that we can get this merged quicker.
Pulling this out from the PR with the design doc (#6138 ) so that we can get this merged quicker.
This was pulled out and added in this PR: - #6592
This is a draft design doc for what we've been calling the automated "flag for manual review" process. It is not strictly a design doc since I ended up creating the solution ahead of time as I was experimenting with Github's API and Circle's CI config, but I tried to capture some design decisions and open questions.
All feedback welcome.
Here is an earlier issue which describes some of the goals. I will try to work that into the design doc somehow.