mirror of
https://github.com/sourcegraph/sourcegraph.git
synced 2026-02-06 13:31:54 +00:00
Move development docs from handbook to doc/dev (#18430)
* Move development docs from handbook to doc/dev * Update codenotify * Fix broken links * Move language guides from handbook to here
This commit is contained in:
Thorsten Ball
GitHub
parent
9aae1086cb
commit
6ff28bd6e8
@ -1,2 +1,3 @@
|
||||
**/* @christinaforney
|
||||
**/admin/** @sourcegraph/distribution
|
||||
dev/background-information/adding_ping_data.md @ebrodymoore @dadlerj
|
||||
|
||||
72
doc/dev/background-information/adding_ping_data.md
Normal file
72
doc/dev/background-information/adding_ping_data.md
Normal file
@ -0,0 +1,72 @@
|
||||
# Adding, changing and debugging pings
|
||||
|
||||
This page outlines the process for adding or changing the data collected from Sourcegraph instances through pings.
|
||||
|
||||
## Ping philosophy
|
||||
|
||||
Pings are the only data Sourcegraph receives from installations. Our users and customers trust us with their most sensitive data. We must preserve and build this trust through only careful additions and changes to pings.
|
||||
|
||||
All ping data must be:
|
||||
|
||||
- Anonymous (with only one exception—the email address of the initial site installer)
|
||||
- Aggregated (e.g. number of times a search filter was used per day, instead of the actual search queries)
|
||||
- Non-specific (e.g. no repo names, no usernames, no file names, no specific search queries, etc.)
|
||||
|
||||
## Adding data to pings
|
||||
|
||||
Treat adding new data to pings as having a very high bar. Would you be willing to send an email to all Sourcegraph users explaining and justifying why we need to collect this additional data from them? If not, don’t propose it.
|
||||
|
||||
1. Write an RFC describing the problem, data that will be added, and how Sourcegraph will use the data to make decisions. The BizOps team must be a required reviewer (both @Dan and @EricBM). Please use [these guidelines](https://about.sourcegraph.com/handbook/ops/bizops/index.md#submitting-a-data-request) and the following questions to inform the contents of the RFC:
|
||||
- Why was this particular metric/data chosen?
|
||||
- What business problem does collecting this address?
|
||||
- What specific product or engineering decisions will be made by having this data?
|
||||
- Will this data be needed from every single installation, or only from a select few?
|
||||
- Will it be needed forever, or only for a short time? If only for a short time, what is the criteria and estimated timeline for removing the data point(s)?
|
||||
- Have you considered alternatives? E.g., collecting this data from Sourcegraph.com, or adding a report for admins that we can request from some number of friendly customers?
|
||||
1. When the RFC is approved, use the [life of a ping documentation](https://docs.sourcegraph.com/dev/background-information/architecture/life-of-a-ping) with help of [an example PR](https://github.com/sourcegraph/sourcegraph/pull/15389) to implement the change. At least one member of the BizOps team must approve the resulting PR before it can be merged. DO NOT merge your PR yet. Steps 3, 4, and 5 must be completed before merging.
|
||||
- Ensure a CHANGELOG entry is added, and that the two sources of truth for ping data are updated along with your PR:
|
||||
- Pings documentation: https://docs.sourcegraph.com/admin/pings
|
||||
- The Site-admin > Pings page, e.g.: https://sourcegraph.com/site-admin/pings
|
||||
1. Determine if any transformations/ETL jobs are required, and if so, add them to the [script](https://github.com/sourcegraph/analytics/blob/master/BigQuery%20Schemas/transform.js). The script is primarily for edge cases. Primarily, as long as zeroes or nulls are being sent back instead of `""` in the case where the data point is empty.
|
||||
1. Open a PR to change [the schema](https://github.com/sourcegraph/analytics/tree/master/BigQuery%20Schemas) with BizOps (EricBM and Dan) as approvers. Keep in mind:
|
||||
- Check the data types sent in the JSON match up with the BigQuery schema (e.g. a JSON '1' will not match up with a BigQuery integer).
|
||||
- Every field in the BigQuery schema should not be non-nullable (i.e. `"mode": "NULLABLE"` and `"mode": "REPEATED"` are acceptable). There will be instances on the older Sourcegraph versions that will not be sending new data fields, and this will cause pings to fail.
|
||||
1. Once the schema change PR is merged, test the new schema. Contact @EricBM for this part.
|
||||
- Delete the [test table](https://bigquery.cloud.google.com/table/telligentsourcegraph:sourcegraph_analytics.update_checks_test?pli=1) (`$DATASET.$TABLE_test`), create a new table with the same name (`update_checks_test`), and then upload the schema with the newest version (see "Changing the BigQuery schema" for commands). This is done to wipe the data in the table and any legacy configurations that could trigger a false positive test, but keep the connection with Pub/Sub.
|
||||
- Update and publish [a message](https://github.com/sourcegraph/analytics/blob/bfe437c92456f5ddb3c0e765e14133e1e1604bfb/BigQuery%20Schemas/pubsub_message.json) to [Pub/Sub](https://console.cloud.google.com/cloudpubsub/topic/detail/server-update-checks-test?project=telligentsourcegraph), which will go through [Dataflow](https://console.cloud.google.com/dataflow/jobs/us-central1/2020-02-28_09_44_54-15810172927534693373?project=telligentsourcegraph&organizationId=1006954638239) to the BigQuery test table. The message can use [this example](https://github.com/sourcegraph/analytics/blob/master/BigQuery%20Schemas/pubsub_message) as a baseline, and add sample data for the new ping data points.
|
||||
- To see if it worked, go to the [`update_checks_test`](https://bigquery.cloud.google.com/table/telligentsourcegraph:sourcegraph_analytics.update_checks_test?pli=1) table, and run a query against it checking for the new data points. Messages that fail to publish are added to the [error records table](https://bigquery.cloud.google.com/table/telligentsourcegraph:sourcegraph_analytics.update_checks_test_error_records?pli=1).
|
||||
1. Merge the PR
|
||||
|
||||
## Changing the BigQuery schema
|
||||
|
||||
Commands:
|
||||
|
||||
- To update schema: `bq --project_id=$PROJECT update --schema $SCHEMA_FILE $DATASET.$TABLE`, replacing `$PROJECT` with the project ID, `$SCHEMA_FILE` with the path to the schema JSON file generated above, and `$DATASET.$TABLE` with the dataset and table name, separated by a dot.
|
||||
- To retrieve the current schema : `bq --project_id=$PROJECT --format=prettyjson show $DATASET.$TABLE > schema.json` with the same replacements as above.
|
||||
|
||||
To update the schema:
|
||||
1. Run the update schema command on a test table.
|
||||
2. Once the test is complete, run the update schema command on the production table.
|
||||
|
||||
## Changing the BigQuery scheduled queries
|
||||
|
||||
1. Add the fields you'd like to bring into BigQuery/Looker to the [instances scheduled queries 1 and 2](https://bigquery.cloud.google.com/scheduledqueries/telligentsourcegraph).
|
||||
2. If day-over-day (or similar) data is necessary, create a new table/scheduled query. For example, daily active users needs a [separate table](https://bigquery.cloud.google.com/table/telligentsourcegraph:sourcegraph_analytics.server_daily_usage) and [scheduled query](https://bigquery.cloud.google.com/scheduledqueries/telligentsourcegraph/location/us/runs/5c51773a-0000-2fc8-bf1f-089e08266748).
|
||||
|
||||
## Debugging pings
|
||||
|
||||
Options for debugging ping abnormalities. Refer to [life of a ping](https://docs.sourcegraph.com/dev/background-information/architecture/life-of-a-ping) for the steps in the ping process.
|
||||
|
||||
1. BigQuery: Query the [update_checks error records](https://console.cloud.google.com/bigquery?sq=839055276916:62219ea9d95d4a49880e661318f419ba) and/or [check the latest pings received](https://console.cloud.google.com/bigquery?sq=839055276916:3c6a5282e66a4f0fac1b958305d7b197) based on installer email admin.
|
||||
1. Dataflow: Review [Dataflow](https://console.cloud.google.com/dataflow/jobs/us-central1/2020-02-05_10_31_47-13247700157778222556?project=telligentsourcegraph&organizationId=1006954638239): WriteSuccessfulRecords should be full of throughputs and the Failed/Error jobs should be empty of throughputs.
|
||||
1. Stackdriver (log viewer): [Check the frontend logs](https://console.cloud.google.com/logs/viewer?project=sourcegraph-dev&minLogLevel=0&expandAll=false&customFacets=&limitCustomFacetWidth=true&interval=PT1H&resource=k8s_container%2Fcluster_name%2Fdot-com%2Fnamespace_name%2Fprod%2Fcontainer_name%2Ffrontend), which contain all pings that come through Sourcegraph.com. Use the following the advanced filters to find the pings you're interested in.
|
||||
1. Grafana: Run `src_updatecheck_client_duration_seconds_sum` on [Grafana](https://sourcegraph.com/-/debug/grafana/explore?orgId=1&left=%5B%22now-1h%22,%22now%22,%22Prometheus%22,%7B%7D,%7B%22ui%22:%5Btrue,true,true,%22none%22%5D%7D%5D) to understand how long each method is taking. Request this information from an instance admin, if necessary.
|
||||
1. Test on a Sourcegraph dev instance to make sure the pings are being sent properly
|
||||
|
||||
```
|
||||
resource.type="k8s_container"
|
||||
resource.labels="dot-com"
|
||||
resource.labels.cluster_name="prod"
|
||||
resource.labels.container_name="frontend"
|
||||
"[COMPANY]" AND "updatecheck"
|
||||
```
|
||||
@ -0,0 +1,44 @@
|
||||
# Introducing a new service
|
||||
|
||||
Before reading this document be sure to first check out our [architecture overview](https://docs.sourcegraph.com/dev/background-information/architecture).
|
||||
|
||||
## Terminology
|
||||
|
||||
When we say "service" here we are referring to code that runs _within a Docker container_. This may be one or more processes, goroutines, threads, etc. operating in a single Docker container.
|
||||
|
||||
## When does introducing a new service make sense?
|
||||
|
||||
Sourcegraph is composed of several smaller services (gitserver, searcher, symbols, etc.) and a single monolithic service (the frontend).
|
||||
|
||||
When thinking of adding a new service, it is important to think through the following questions carefully:
|
||||
|
||||
1. Does the code belong in an existing service?
|
||||
2. Instead of introducing a new service, could the process/etc reasonably live inside another service (the frontend, etc.) as a background worker, goroutine, etc.?
|
||||
3. Is the code heavily coupled to an existing service logic? ie. Changes to the other service will likely require changes to this service. If so, have you considered putting it in that service instead?
|
||||
4. If you make the change within an existing service, instead of a new one, would it substantially increase the complexity of the task at hand?
|
||||
- For example, the service you are writing _must_ be written in language X and it is impossible/very difficult to integrate language X into one of our existing Go services.
|
||||
5. Does it need its own resource constraints and scaling?
|
||||
- For example, the service you are creating needs its own CPU / memory resource constraints, or must be able to scale horizontally across machines.
|
||||
|
||||
If after asking the above questions to yourself you still believe introducing a new service is the best approach forward, you should [create an RFC](https://about.sourcegraph.com/handbook/engineering/rfcs) proposing it to the rest of the team. In your RFC, be sure to answer the above questions to explain why you believe a separate service is a better choice than integration into an existing service.
|
||||
|
||||
### Services have additional overhead for us and users that is easy to forget
|
||||
|
||||
Introducing a new service means additional complexity and overhead for us and users. While they may provide a good way to isolate code, services are very much not free. Of course, a monolith is not free or necessarily cheaper either. The pros and cons _in our situation, not based on preference_ must be weighed.
|
||||
|
||||
When introducing a new service/container we pay the cost of:
|
||||
|
||||
- Introducing and maintaining [its Kubernetes YAML](https://github.com/sourcegraph/deploy-sourcegraph/tree/master/base).
|
||||
- Adding it to our [docker-compose deployments](https://github.com/sourcegraph/deploy-sourcegraph-docker/pull/38).
|
||||
- Integrating it as a raw process in [the single-container `sourcegraph/server` deployment mode](https://github.com/sourcegraph/sourcegraph/tree/master/cmd/server).
|
||||
- Documenting clearly [how it scales](https://docs.sourcegraph.com/admin/install/kubernetes/scale) alongside other services for cluster deployments.
|
||||
- Updating our [architecture diagram](https://docs.sourcegraph.com/dev/background-information/architecture).
|
||||
- Documenting the service itself in general and how site admins should manage and debug it (these needs to be done regardless of it being a new service, but if it is a new service there are additional aspects to consider.)
|
||||
- [Updating deploy-sourcegraph-docker](https://github.com/sourcegraph/deploy-sourcegraph-docker) - including testing that it works, documenting which other services it speaks to, and notifying the customers relying on this deployment documentation to deploy this service on their own.
|
||||
- Note: We must advise these customers **exactly** which new container has been added, how to deploy it and with what configuration, describe what it does, why we've added it, and assist with that process. Remember, these users are not just running the scripts in our repository -- they are effectively deploying these containers arbitrarily on their own infrastructure with our guidance.
|
||||
- Updating the [resource estimator](https://docs.sourcegraph.com/admin/install/resource_estimator) to provide details on resource requirements at different scales.
|
||||
- Training new and existing Sourcegraph team members how to interact with and debug the service, as well as customers and our Customer Engineering team (this needs to happen for the feature/change regardless, but as a new service there are some additional aspects.)
|
||||
|
||||
Do not introduce a new service/container just for sake of code seperation. Instead, look for alternatives that allow you to achieve the same logical code seperation within the right existing service/container (goroutines, multiple processes in a container, etc. are all valid options.)
|
||||
|
||||
Introducing a new service/container can make sense in some circumstances, it is very important to weigh the pros and cons of each based on the circumstance and consider if the value gained is worth it or not.
|
||||
136
doc/dev/background-information/code_reviews.md
Normal file
136
doc/dev/background-information/code_reviews.md
Normal file
@ -0,0 +1,136 @@
|
||||
# Code reviews
|
||||
|
||||
All code should be reviewed and approved by an appropriate teammate before being merged into the `main` branch.
|
||||
|
||||
Our goal is to have a code review process and culture that everyone would opt-in to even if code reviews weren't required.
|
||||
|
||||
## Why do we require peer code reviews?
|
||||
|
||||
Code reviews are an investment. As an author, it takes extra time and effort to create good PRs, wait for someone to review your code, and address review comments. As a code reviewer, it takes extra time to understand the motivation, design, and implementation of someone else's code so that you can provide valuable feedback. This investment is worth it because it provides benefits to every person on the team and it helps the team as a whole ship features to our customers that are _high quality_ and _maintainable_.
|
||||
|
||||
For authors:
|
||||
|
||||
- Requesting a code review from one of your peers is motivation to ensure that the quality of your code is high.
|
||||
- Writing a good PR description (and commit messages) develops your technical communication skills.
|
||||
- Receiving code review feedback can give you valuable insight into aspects of your change that you hadn't considered (architectural, performance, etc).
|
||||
- Receiving feedback about how your code could be improved helps you learn how to write better code in the future.
|
||||
|
||||
For reviewers:
|
||||
|
||||
- Reviewing code from others increases your code literacy and empathy. You will learn new things from how your peers write code and you will be more aware of how to write code that is easier for your reviewers to understand.
|
||||
- Reading PR descriptions increases your technical literacy and empathy. You will learn what makes a PR description effective and you will be more aware of how to write PR descriptions that are easier for your reviewers to understand.
|
||||
- Being a reviewer gives you the ability to share knowledge you have that others do not. Every person, regardless of experience level, has both something to learn and something to contribute.
|
||||
|
||||
For the team:
|
||||
|
||||
- Code reviews increase the overall quality of our codebase by catching design and implementation defects before changes ship to customers.
|
||||
- Code reviews increase the overall consistency of our codebase by spreading knowledge of best practices and eliminating anti-patterns.
|
||||
- Code reviews distribute domain expertise, code knowledge, and ownership across the team so development can continue when people are unavailable (e.g. vacation).
|
||||
|
||||
## Review cycles
|
||||
|
||||
Once a change is ready for review it may go through a few review cycles, in general it'll look something like this:
|
||||
|
||||
1. Ready for review
|
||||
1. Reviewers leave feedback
|
||||
1. Author addresses feedback, ensuring they have marked each comment as resolved. This can be relaxed if a comment was just a suggestion or a small nit. The goal is to communicate to the reviewer what has changed since they last reviewed.
|
||||
1. Author leaves a comment letting the reviewers know it is ready for another pass. Back to step 1.
|
||||
|
||||
Use your best judgement when deciding if another review cycle is needed. If your PR has been approved and the only changes you've made are ones that the approver would expect: accepting a suggestion around a name, or fleshing out some documentation, or something else that is low risk and you're comfortable with, you may choose to avoid another cycle and merge. However, you can always ask for more feedback if you're not totally comfortable.
|
||||
|
||||
## When is a code review not required?
|
||||
|
||||
We do not technically prevent PRs from being merged before being explicitly approved because there exist PRs that are appropriate to merge without waiting for a review.
|
||||
|
||||
Some examples:
|
||||
|
||||
- Reverting a previous change to solve a production issue.
|
||||
- Minor documentation changes.
|
||||
- Auto-generated changes, such as when performing a version release, where a reviewer would not have anything of substance to review.
|
||||
|
||||
It should be obvious to any other engineer on the team from the PR description and/or diff that it was appropriate for you to not wait for approval.
|
||||
|
||||
Here are some examples of reasons to skip code review that are NOT acceptable:
|
||||
|
||||
- "I promised that I would ship this to $CUSTOMER by $DEADLINE"
|
||||
- The customer expects the feature to work and be maintained. Code review helps ensure both of these things by increasing the quality and distributing ownership.
|
||||
- "This code is experimental"
|
||||
- Our goal is to have a code review culture such that engineers who are working on "experimental" code still find code reviews valuable and worth doing (for all the benefits mentioned in the rest of this document).
|
||||
- All code that is in `main` has the potential to impact customers (e.g. by causing a bug) and other developers at Sourcegraph (e.g. by making it harder to refactor code). As such, it is in our interest to ensure a certain quality level on all code whether or not it is considered "experimental".
|
||||
- Assume that we allowed "experimental" code to bypass code review. How would we know when it is no longer experimental and how would it get reviewed? Either it wouldn't get reviewed, or an engineer would have to review all the code after the fact without a nice PR diff to look at or effective way to make comments. Neither of these outcomes would meet our need of reviewing all non-experimental code.
|
||||
- "I don't have someone to review this code"
|
||||
- Ask for help to identify someone else on the team with whom you can share your knowledge, context, and ownership.
|
||||
|
||||
If we see that there are too many [PRs being merged without approval](https://github.com/pulls?page=1&q=is%3Apr+org%3Asourcegraph+is%3Amerged+review%3Anone+-author%3Aapp%2Frenovate&utf8=%E2%9C%93) that lack an appropriate reason, then we will have to consider an automated solution to enforce code reviews.
|
||||
|
||||
## What makes an effective Pull Request (PR)?
|
||||
|
||||
An effective PR minimizes the amount of effort that is required for the reviewer to understand your change so that they can provide high quality feedback in a timely manner. [Further reading](https://www.atlassian.com/blog/git/written-unwritten-guide-pull-requests).
|
||||
|
||||
Do:
|
||||
|
||||
- Prefer small PRs. If a large PR is unavoidable, try to organize it into smaller commits (with [good commit messages](commit_messages.md)) that are reviewable independently (and indicate this to the reviewer in the description).
|
||||
- Create a draft PR first and review your own diff as if you were reviewing someone else's change. This helps you empathize with your reviewer and can help catch silly mistakes before your reviewer sees them (e.g. forgetting to `git add` a file, forgetting to remove debugging code, etc.).
|
||||
- Create meaningful PR title and description that communicates **what** the PR does and **why** (the **how** is the diff).
|
||||
- Include links to relevant issues (e.g. "closes #1234", "helps #5678").
|
||||
- Include a screenshot, GIF, or video if you are making user facing changes.
|
||||
- Discuss alternative implementations that you considered but didn't pursue (and why).
|
||||
- Describe any remaining open questions that you are seeking feedback on.
|
||||
- Make it clear whose review(s) you are waiting for.
|
||||
- Politely remind your reviewer that you are waiting for a review if they haven't responded after one business day.
|
||||
- If you need to work on your PR after a first round of reviews, try not to force push commits that erase the latest reviewed commit or any commit prior to that. This makes it easy for reviewers to pick up where they left off in the previous round.
|
||||
|
||||
## What makes an effective code review?
|
||||
|
||||
Please read:
|
||||
|
||||
- [On code reviews](https://medium.com/@schrockn/on-code-reviews-b1c7c94d868c)
|
||||
- [Code Health: Respectful Reviews == Useful Reviews](https://testing.googleblog.com/2019/11/code-health-respectful-reviews-useful.html)
|
||||
|
||||
The code author and code reviewer have a _shared goal_ to bring the PR into a state that meets our quality standards and the needs that motivated the change.
|
||||
|
||||
Do:
|
||||
|
||||
- Take time to understand the context and goal of the PR. If it isn't clear, ask for clarification.
|
||||
- Acknowledge what the author did well to balance the tone of the review.
|
||||
- Make it clear which comments are blocking your explicit approval (e.g. use "nit:" prefix for minor comments) and approve if all of your comments are minor.
|
||||
- If the author were to address all of your comments faithfully and you would be content, then you should also approve to avoid the author needing to wait for a subsequent review without reason (exception: you asked for fundamental or vast/large changes and believe those will need re-review by you).
|
||||
- When you are making comments on a PR, use a tone that is kind, empathetic, collaborative, and humble. [Further reading](https://mtlynch.io/human-code-reviews-1/).
|
||||
|
||||
| 😕 | 🤗 | Why? |
|
||||
| ----------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| You misspelled "successfully" | sucessfully -> successfully | Avoid using "you" because it can make the comment feel like a personal attack instead of just a minor correction. |
|
||||
| Move the `Foo` class to a separate file. | Can we move the `Foo` class to a separate file to avoid large files that are hard to scan? | Feedback that is framed as a request encourages collaborative conversation. Including your reason helps the author understand the motivation behind your request. |
|
||||
| Can we simplify this with a list comprehension? | Consider simplifying with a list comprehension like this: $EXAMPLE or $LINK_TO_EXAMPLE | Providing example code (using GitHub suggestions) or a link to an example helps the author quickly understand and apply your recommended change. This is particularly helpful when the author might not be familiar with concept that you are describing. |
|
||||
| Why didn't you \$X? | What do you think about doing $X? I think it would help with $Y. | A "What" question is better than a "Why" question because the latter sounds accusatory. Either the author considered $X and decided not to, or they didn't consider it, but in either case it is frustrating to the author if the reviewer doesn't explain why they think $X should be considered. |
|
||||
| This code is confusing. Can you clarify it? | It was hard for me to understand $X because $Y, but I haven't been able to think of a way to improve it. Do you have any ideas? | Don't declare your opinion as fact; instead, share your subjective experience and try to offer a suggestion for improvement. If you don't have a suggestion, acknowledge that to show that you empathize with the difficulty of improving this code. |
|
||||
|
||||
Don't:
|
||||
|
||||
- Comment on the details of the PR if you have questions or concerns about the overall direction or design. The former will distract from the latter and might be irrelevant if the PR is reworked.
|
||||
- Take longer than one business day to respond to a PR that is ready for your review (or re-review).
|
||||
- Have protracted discussions in PR comments. If it can't be settled quickly in a few review round trips, try discussing in person or on a video call because these mediums are higher bandwidth and encourage empathy. Then summarize the results in the PR discussion after the fact.
|
||||
|
||||
## GitHub notifications
|
||||
|
||||
Ensure that you have you [GitHub notification settings](https://about.sourcegraph.com/handbook/engineering/github-notifications) configured correctly so that you are responsive to comments on PRs that you have authored and to review requests from teammates.
|
||||
|
||||
## Who should I get a code review from?
|
||||
|
||||
You should get a code review from the person who's approval will give you the most confidence that your change is high quality. If you are modifying existing code, you can use `git blame` to identify the previous author and the previous reviewer because they probably will have helpful context.
|
||||
|
||||
If your change touches multiple parts of our codebase (e.g. Go, TypeScript), then you will need to get approval from multiple peers (e.g. a Go reviewer and a TypeScript reviewer).
|
||||
|
||||
GitHub will automatically assign reviewers if there is a matching entry in the [CODEOWNERS](https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/.github/CODEOWNERS) file, but that doesn't necessarily mean that you need to wait for an approval from everyone. For example, if you are making a change to the search backend then you only need approval from one person on that team, not all of them.
|
||||
|
||||
For small changes across few files assign to the team and request a review.
|
||||
|
||||
Use your best judgement and ask if you are uncertain.
|
||||
|
||||
## When should I use a draft PR?
|
||||
|
||||
A draft PR signals that the change is not ready for reviewed. This is useful, for example, if you want to self-review your diff before sending review requests to others. If you are looking for feedback or discussion on your change, then you should mark the PR as ready for review and communicate your intentions in the PR description.
|
||||
|
||||
## Security
|
||||
|
||||
Special care should be taken when reviewing a diff that contains, or is adjacent to, comments that contain the following string: `🚨 SECURITY`. These comments indicate security-sensitive code, the correctness of which is necessary to ensure that no private data is accessed by unauthorized actors. The code owner of any modified security-sensitive code must approve the changeset before it is merged. Please refer to [Security patterns](security_patterns.md) for considerations when touching security-sensitive code.
|
||||
35
doc/dev/background-information/commit_messages.md
Normal file
35
doc/dev/background-information/commit_messages.md
Normal file
@ -0,0 +1,35 @@
|
||||
# Commit message guidelines
|
||||
|
||||
This document contains recommendations on how to write good commit messages. Having consistent commit messages makes it easier to see at a glance what code has been affected by recent changes. It also makes it easier to search through our commit history.
|
||||
|
||||
To get some background, please read [How to Write a Git Commit Message](https://chris.beams.io/posts/git-commit/)
|
||||
|
||||
These guidelines are not hard requirements and not enforced in any way. If any of these guidelines turn in to hard requirements in the future, they must be enforced by automated tooling.
|
||||
|
||||
## Format
|
||||
|
||||
A commit message has two parts: a subject and a body. These are separated by a blank line.
|
||||
|
||||
### Subject
|
||||
|
||||
The subject line should be concise and easy to visually scan in a list of commits, giving context around what code has changed.
|
||||
|
||||
1. Prefix the subject with the primary area of code that was affected (e.g. `web:`, `cmd/searcher:`).
|
||||
2. Limit the subject line to 50 characters.
|
||||
3. Do not end the subject line with punctuation.
|
||||
4. Use the [imperative mood](https://chris.beams.io/posts/git-commit/#imperative) in the subject line.
|
||||
|
||||
| Prefer | Instead of |
|
||||
|--------|------------|
|
||||
| Fix bug in XYZ | Fixed a bug in XYZ |
|
||||
| Change behavior of X | Changing behavior of X |
|
||||
|
||||
Example:
|
||||
|
||||
> cmd/searcher: Add scaffolding for structural search
|
||||
|
||||
## Body
|
||||
|
||||
The purpose of a commit body is to explain _what_ changed and _why_. The _how_ is the diff.
|
||||
|
||||
[Code review](code_reviews.md) happens in PRs, which should contain a good subject and description. When a PR is approved, we prefer to squash merge all commits on the PR branch into a single commit on master. After clicking "Squash and merge", edit the body of the final commit message so that it is clean and informative. The commit body should either be empty (assuming that you have a good PR description), or a brief summary of your change (which could be exactly your PR description if it is concise). It should not include unimportant details from incremental commits on the PR branch (e.g. `* add test`, `* fix test`, `* try X`, `Update filename.md`, `Co-Authored-By...`).
|
||||
35
doc/dev/background-information/continuous_integration.md
Normal file
35
doc/dev/background-information/continuous_integration.md
Normal file
@ -0,0 +1,35 @@
|
||||
# Continuous Integration
|
||||
|
||||
We have a variety of tooling on [Buildkite](https://buildkite.com/sourcegraph/sourcegraph) and [GitHub Actions](https://github.com/sourcegraph/sourcegraph/actions) for continuous integration.
|
||||
|
||||
- [GitHub Actions](#github-actions)
|
||||
- [Third-Party Licenses](#third-party-licenses)
|
||||
|
||||
## GitHub Actions
|
||||
|
||||
### Third-Party Licenses
|
||||
|
||||
We use the [`license_finder`](https://github.com/pivotal/LicenseFinder) tool to check third-party dependencies for their licenses. It runs as a [GitHub Action on pull requests](https://github.com/sourcegraph/sourcegraph/actions?query=workflow%3A%22Licenses+Check%22), which will fail if one of the following occur:
|
||||
|
||||
- If the license for a dependency cannot be inferred. To resolve:
|
||||
- Use `license_finder licenses add <dep> <license>` to set the license manually
|
||||
- If the license for a new or updated dependency is not on the list of approved licenses. To resolve, either:
|
||||
- Remove the dependency
|
||||
- Use `license_finder ignored_dependencies add <dep> --why="Some reason"` to ignore it
|
||||
- Use `license_finder permitted_licenses add <license> --why="Some reason"` to allow the offending license
|
||||
|
||||
The `license_finder` tool can be installed using `gem install license_finder`. You can run the script locally using:
|
||||
|
||||
```sh
|
||||
# updates ThirdPartyLicenses.csv
|
||||
./dev/licenses.sh
|
||||
|
||||
# runs the same check as the one used in CI, returning status 1
|
||||
# if there are any unapproved dependencies ('action items')
|
||||
LICENSE_CHECK=true ./dev/licenses.sh
|
||||
```
|
||||
|
||||
The `./dev/licenses.sh` script will also output some `license_finder` configuration for debugging purposes - this configuration is based on the `doc/dependency_decisions.yml` file, which tracks decisions made about licenses and dependencies.
|
||||
|
||||
For more details, refer to the [`license_finder` documentation](https://github.com/pivotal/LicenseFinder#usage).
|
||||
|
||||
30
doc/dev/background-information/exposing-services.md
Normal file
30
doc/dev/background-information/exposing-services.md
Normal file
@ -0,0 +1,30 @@
|
||||
# Exposing services
|
||||
|
||||
In Go, that looks like this:
|
||||
```
|
||||
http.ListenAndServe(":80", nil)
|
||||
```
|
||||
The above code will bind to all TCP interfaces. Since Kubernetes does support dual-stack IPv4 & IPv6 our services should bind to all interfaces (we do not currently have IPv6 services).
|
||||
|
||||
If you must specify an ip address to expose your service on, choosing `0.0.0.0:port` is typically a good choice. What this does can be OS and platform-specific.
|
||||
|
||||
Binding to `localhost:port` or `127.0.0.1:port` is binding to a local-only interface that constrained to the same "host". In Kubernetes, other containers within the Pod may still communicate with this service AND you may port-forward this container and associated service
|
||||
[(Why?)](#How-can-I-port-forward-a-local-only-service?).
|
||||
|
||||
This may be preferred in a sidecar pattern where you do not want a container accessible outside a Pod or when you don't want expose your laptop to the cofeeshop wifi but generally, code should not be merged that binds to `localhost` or `127.0.0.1`.
|
||||
|
||||
### How can I port-forward a local only service?
|
||||
|
||||
Due to the way that kube-proxy works when you port-forward a pod or a service kube-proxy opens a tunnel to that pod (or a pod that backs that service). This can make debugging why a service is accessible in the pod but not outside of the pod hard to [understand](https://github.com/sourcegraph/zoekt/pull/46/files).
|
||||
|
||||
You can also use `kubectl port-forward --address 0.0.0.0` if you need to expose a port-forwarded service outside of your local machine (it defaults to `127.0.0.1`). [link](https://github.com/kubernetes/kubernetes/issues/40053) :exploding_head:
|
||||
|
||||
### CI
|
||||
|
||||
We also have CI test for this [here](https://sourcegraph.com/github.com/sourcegraph/sourcegraph@main/-/blob/dev/check/no-localhost-guard.sh)
|
||||
|
||||
### References
|
||||
|
||||
- https://stackoverflow.com/questions/20778771/what-is-the-difference-between-0-0-0-0-127-0-0-1-and-localhost
|
||||
- https://serverfault.com/questions/21657/semantics-of-and-0-0-0-0-in-dual-stack-oses/39561#39561
|
||||
- https://stackoverflow.com/questions/49067160/what-is-the-difference-in-listening-on-0-0-0-080-and-80
|
||||
12
doc/dev/background-information/honeycomb.md
Normal file
12
doc/dev/background-information/honeycomb.md
Normal file
@ -0,0 +1,12 @@
|
||||
# Honeycomb
|
||||
|
||||
Honeycomb is a service we have enabled on Sourcegraph.com to allow Sourcegraph engineers to diagnose issues in production.
|
||||
|
||||
> Honeycomb is a tool for introspecting and interrogating your production systems.
|
||||
|
||||
Link: https://ui.honeycomb.io/sourcegraph/datasets/gitserver-exec
|
||||
Login: Ask in #dev-chat for access (cc @keegan).
|
||||
|
||||
In particular we have instrumented the git commands we run. Nearly every user request on Sourcegraph.com involves interacting with a git repository. We send an event to Honeycomb for every git command we run (sampled).
|
||||
|
||||
This allows you to interactively slice, dice and visualize what Sourcegraph is doing. You can quickly narrow down problems like which commands are taking the longest, which repository is having the most commands run against it, etc. I recommend exploring the UI, it is very powerful.
|
||||
@ -3,8 +3,12 @@
|
||||
## Overview
|
||||
|
||||
- [Tech stack](tech_stack.md)
|
||||
- [Architecture](architecture/index.md)
|
||||
- [Testing](testing.md)
|
||||
- [Security Patterns](security_patterns.md)
|
||||
|
||||
## [Architecture](architecture/index.md)
|
||||
|
||||
- [Overview](architecture/index.md)
|
||||
- [Introducing a new service](architecture/introducing_a_new_service.md)
|
||||
|
||||
## Development
|
||||
|
||||
@ -18,12 +22,35 @@
|
||||
- [Developing observability](observability/index.md)
|
||||
- [Developing Sourcegraph extensions](sourcegraph_extensions.md)
|
||||
- [Dependencies and generated code](dependencies_and_codegen.md)
|
||||
- [Code reviews](code_reviews.md)
|
||||
- [Commit messages](commit_messages.md)
|
||||
- [Exposing services](exposing-services.md)
|
||||
|
||||
## [Languages](languages/index.md)
|
||||
|
||||
- [Go](languages/go.md)
|
||||
- [Testing Go code](languages/testing_go_code.md).
|
||||
- [TypeScript](languages/typescript.md)
|
||||
- [Bash](languages/bash.md)
|
||||
- [Terraform](languages/terraform.md)
|
||||
|
||||
### [Extended guides](languages/extended_guide/index.md)
|
||||
|
||||
- [Terraform Extended Guide](languages/extended_guide/terraform.md)
|
||||
|
||||
## Testing
|
||||
|
||||
- [Continuous Integration](continuous_integration.md)
|
||||
- [How to run tests](testing.md)
|
||||
- [Testing Principles](testing_principles.md)
|
||||
|
||||
## Tools
|
||||
|
||||
- [Renovate dependency updates](renovate.md)
|
||||
- [Honeycomb](honeycomb.md)
|
||||
- [Using PostgreSQL](postgresql.md)
|
||||
|
||||
## Other
|
||||
|
||||
- [Telemetry](telemetry.md)
|
||||
- [Adding, changing and debugging pings](adding_ping_data.md)
|
||||
|
||||
49
doc/dev/background-information/languages/bash.md
Normal file
49
doc/dev/background-information/languages/bash.md
Normal file
@ -0,0 +1,49 @@
|
||||
# Bash style
|
||||
|
||||
Bash is frequently used in our build, CI and deployment systems. Some general guidelines and recommended reading
|
||||
|
||||
- [Bash style](#bash-style)
|
||||
- [Reading from a file](#reading-from-a-file)
|
||||
- [Set -eu -o pipefail](#set--eu--o-pipefail)
|
||||
- [References](#references)
|
||||
|
||||
### Reading from a file
|
||||
|
||||
Prefer using `mapfile` instead of `while IFS` to read a file
|
||||
|
||||
```bash
|
||||
mapfile -t myArray < file.txt
|
||||
```
|
||||
|
||||
```bash
|
||||
mapfile -t myArray < <(find -d .)
|
||||
```
|
||||
|
||||
instead of
|
||||
|
||||
```bash
|
||||
input="/path/to/txt/file"
|
||||
while IFS= read -r line
|
||||
do
|
||||
echo "$line"
|
||||
done < "$input"
|
||||
```
|
||||
|
||||
### Set -eu -o pipefail
|
||||
|
||||
This is generally "bash strict mode" and sets
|
||||
|
||||
- `e` exit if error
|
||||
- `u` error on variable unset (and exit)
|
||||
- `-o pipefail` fail if items in the pipe | fail. Bash otherwise continues if error | pass which causes some unexpected behavior.
|
||||
|
||||
Recommend using these at the start of all scripts and specifically disabling if a section of a bash script does not need them (for example, you want to let a pipe fail).
|
||||
|
||||
### Utilities in use
|
||||
|
||||
We use `shfmt` and `shellcheck` for our shell script linters
|
||||
|
||||
## References
|
||||
|
||||
<https://wiki.bash-hackers.org/>
|
||||
<https://www.notion.so/daxmc99/One-liners-Basic-Cheat-sheet-Linux-Command-Library-b3676fb5a49b44f8a507cce0185ca5d7>
|
||||
@ -0,0 +1,5 @@
|
||||
# Extended guide
|
||||
|
||||
For more in-depth guides beyond style
|
||||
|
||||
- [Terraform](terraform.md)
|
||||
@ -0,0 +1,18 @@
|
||||
# Playbooks for Terraform
|
||||
|
||||
## Statefile surgery
|
||||
|
||||
_Note: these are provided as internal reference documents. You should rarely need to use this and please reach out to the distribution team or ask in the #dev-ops channel before performing these steps._
|
||||
|
||||
To debug some more involved terraform problems you may need to directly modify the state file.
|
||||
|
||||
1. Use `TF_DEBUG` environment variable with `WARN`, `DEBUG` logs enabled to locate a possible source of the issue.
|
||||
|
||||
1. Use `terraform state pull` to examine the statefile locally.
|
||||
|
||||
1. If possible, use `terraform state rm`/`terraform state mv X Y` to make the necessary state modifications.
|
||||
Otherwise, manually edit the state file and run `terraform state push` to update the remote state.
|
||||
|
||||
1. In cases where a corrupted state has been pushed, you can pull a previous object version with [gsutil](https://cloud.google.com/storage/docs/using-object-versioning)
|
||||
|
||||
1. File bugs as appropriate, statefile modification should typically not be necessary
|
||||
153
doc/dev/background-information/languages/go.md
Normal file
153
doc/dev/background-information/languages/go.md
Normal file
@ -0,0 +1,153 @@
|
||||
# Go style guide
|
||||
|
||||
This file documents the style used in Sourcegraph's code. For non-code text, see the overall [content guidelines](https://about.sourcegraph.com/handbook/communication/content_guidelines).
|
||||
|
||||
For all things not covered in this document, defer to
|
||||
[Go Code Review Comments](https://code.google.com/p/go-wiki/wiki/CodeReviewComments)
|
||||
and [Effective Go](http://golang.org/doc/effective_go.html).
|
||||
|
||||
We also have subsections here:
|
||||
- [Testing Go Code](#Testing)
|
||||
- [Exposing Services](#exposing-services)
|
||||
|
||||
## Panics
|
||||
|
||||
Panics are used for code paths that should never be reached.
|
||||
|
||||
## Options
|
||||
|
||||
In the general case, when a pointer to an "options" struct is an argument
|
||||
to a function (such as `Get(build BuildSpec, opt *BuildGetOptions) (*Build, Response, error)`,
|
||||
that pointer may be `nil`. When the pointer is nil, the function does its default behavior.
|
||||
If the options struct should not be nil, either make the argument the value instead of a
|
||||
pointer or document it.
|
||||
|
||||
## Pull requests
|
||||
|
||||
Avoid unnecessary formatting changes that are unrelated to your change. If you find code that isn't formatted correctly, fix the formatting in a separate PR by running the appropriate formatter (e.g. `prettier`, `gofmt`).
|
||||
|
||||
## Group code blocks logically
|
||||
|
||||
Declare your variables close to where they are actually used.
|
||||
|
||||
prefer
|
||||
|
||||
```go
|
||||
a, b := Vector{1, 2, 3}, Vector{4, 5, 6}
|
||||
a, b := swap(a, b)
|
||||
|
||||
c := Vector{7, 8, 9}
|
||||
total := add([]Vector(a, b, c))...)
|
||||
```
|
||||
|
||||
over
|
||||
|
||||
```go
|
||||
a, b, c := Vector{1, 2, 3}, Vector{4, 5, 6}, Vector{7, 8, 9}
|
||||
a, b := swap(a, b)
|
||||
|
||||
total := add([]Vector(a, b, c))...)
|
||||
```
|
||||
|
||||
The `c` `Vector` isn't used until the `add()` function call, so why not declare it immediately beforehand?
|
||||
|
||||
By logically grouping components together, you make sure that the context around them isn't lost by the time they come into play. More concretely:
|
||||
|
||||
- You have to keep less in your mental buffer -- which is great if you use a screenreader
|
||||
- You have to navigate around the code base less to find definitions or declarations -- and that’s great if you have difficulties with fine motor control
|
||||
- You’re minimizing the amount of navigation needed to comprehend a block of code.
|
||||
|
||||
This advice also goes for other types of declarations -- interfaces, structs, etc…
|
||||
|
||||
## Keep names short
|
||||
|
||||
Prefer
|
||||
|
||||
```go
|
||||
var a, b Vector
|
||||
```
|
||||
|
||||
over
|
||||
|
||||
```go
|
||||
var vectorA, vectorB Vector
|
||||
```
|
||||
|
||||
Go [already encourages short variable names](https://github.com/golang/go/wiki/CodeReviewComments#variable-names).
|
||||
|
||||
In addition, short names:
|
||||
|
||||
- Are faster to listen to (and read)
|
||||
- Are easier to navigate around
|
||||
- Are less effort to type
|
||||
|
||||
In the above example, you might think that the names `vectorA` and `vectorB` are good because you're putting context inside the name itself. That way, there's no confusion / ambiguity when the variables are referred to elsewhere. However, this is redundant / not necessary if you're following the [group code blocks logically](#group-code-blocks-logically) advice above.
|
||||
|
||||
## Make names meaningful
|
||||
|
||||
Prefer
|
||||
|
||||
```go
|
||||
var total, scaled Vector
|
||||
```
|
||||
|
||||
over
|
||||
|
||||
```go
|
||||
var tVec, sVec Vector
|
||||
```
|
||||
|
||||
Using meaningful names reduces the amount of work that a person has to do to understand what’s going on in your code. More concretely:
|
||||
|
||||
- They don’t have to keep as much context in their head about what that variable does.
|
||||
- They don’t have to jump around to find definitions, usage, etc…
|
||||
- It can also help distinguish important variables from temporary placeholders
|
||||
|
||||
Whenever possible, prefer meaningful names over explanatory comments. Comments are an extra thing to navigate around, and they don't actually reduce the amount of jumping around the codebase that you'll need to do when the variables are used later on.
|
||||
|
||||
## Use pronounceable names
|
||||
|
||||
Prefer
|
||||
|
||||
```go
|
||||
var total Vector
|
||||
func add(...)
|
||||
```
|
||||
|
||||
over
|
||||
|
||||
```go
|
||||
var tVec Vector
|
||||
var addAllVecs(...)
|
||||
```
|
||||
|
||||
Pronounceable names:
|
||||
|
||||
- Screen readers can actually read them
|
||||
- Takes less time than pronouncing a string of letters
|
||||
|
||||
[You should watch this short YouTube video of @juliaferraioli navigating some Go code with a screenreader.](https://www.youtube.com/watch?v=xwjvufcJK-Q)
|
||||
|
||||
Now, something you may have noticed during the demo, is how screen readers handle variable names. It’s rough, right?
|
||||
|
||||
[@juliaferraioli](https://twitter.com/juliaferraioli) shared an anecdote about how she spent about 15 minutes scratching her head during a code review the other day, wondering what “gee-thub” was, before she realized that it was reading “GitHub”.
|
||||
|
||||
So make sure you use pronounceable names. Don’t make up words. Think of how they would be spoken. **Avoid concatenated variable names when possible.** Various screen readers won’t necessarily make it clear that the variable name is one word.
|
||||
|
||||
## Use new lines intentionally
|
||||
|
||||
```go
|
||||
a, b := Vector{1, 2, 3}, Vector{4, 5, 6}
|
||||
a, b := swap(a, b)
|
||||
|
||||
c := Vector{7, 8, 9}
|
||||
total := add([]Vector(a, b, c))...)
|
||||
```
|
||||
|
||||
If we revisit the recommended organization, we can also see the usage of new lines. Newlines are something we kind of pepper in our code without really thinking about it. However, they can be really powerful signals. I recommend that you treat them like paragraph breaks -- if you don’t use any at all, your reader is lost. If you use them too much, your message is fragmented. They can help guide the user to where the logical components are.
|
||||
|
||||
Be intentional with them!
|
||||
|
||||
## Testing
|
||||
|
||||
See guidelines for [testing Go code](./testing_go_code.md).
|
||||
8
doc/dev/background-information/languages/index.md
Normal file
8
doc/dev/background-information/languages/index.md
Normal file
@ -0,0 +1,8 @@
|
||||
# Languages programming guides
|
||||
|
||||
Documentation of our principles for writing code in specific programming languages.
|
||||
|
||||
- [Bash](bash.md)
|
||||
- [For TypeScript code](typescript.md)
|
||||
- [For Go code](go.md)
|
||||
- [For Terraform code](terraform.md)
|
||||
89
doc/dev/background-information/languages/terraform.md
Normal file
89
doc/dev/background-information/languages/terraform.md
Normal file
@ -0,0 +1,89 @@
|
||||
# Terraform style guide
|
||||
|
||||
- General Terraform [styleguide](https://www.terraform.io/docs/configuration/style.html)
|
||||
- Sourcegraph Terraform [Extended Guide](./extended_guide/terraform.md)
|
||||
|
||||
## State
|
||||
|
||||
State must be stored using a [GCS Terraform state backend](https://www.terraform.io/docs/backends/types/gcs.html).
|
||||
|
||||
Example configuration
|
||||
```
|
||||
terraform {
|
||||
required_version = "0.12.26"
|
||||
|
||||
backend "gcs" {
|
||||
bucket = "sourcegraph-tfstate"
|
||||
prefix = "infrastructure/dns"
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### State for state buckets
|
||||
|
||||
Because we need to create state buckets as code, we also need to store the state of the code that creates the state bucket. Given this code rarely changes and that moving it to be stored in a remote location creates a chicken and egg situation, we will store state bucket creation's state in Git.
|
||||
|
||||
### Bucket
|
||||
|
||||
State for all Sourcegraph resources is stored in [sourcegraph-tfstate bucket](https://github.com/sourcegraph/infrastructure/tree/master/terraform-state).
|
||||
|
||||
Managed instances resources will be stored on a per customer bucket following the pattern: `sourcegraph-managed-${NAME}`.
|
||||
|
||||
### Prefix
|
||||
|
||||
State for different pieces of infrastructure require separate state files. To facilitate matching state to resources and code, we will use the following pattern: `${REPOSITORY_NAME}/${INFRASTRUCTURE_NAME}`.
|
||||
|
||||
## Formatting
|
||||
|
||||
- Format all code using `terraform fmt`
|
||||
- Remove duplicate empty new lines
|
||||
> Terraform > `0.12`, the `fmt` command lost the ability (as it was too aggressive) to remove multiple empty new-lines, etc.
|
||||
|
||||
## General
|
||||
|
||||
- Use **lowercase**
|
||||
- Use [snake_case](https://en.wikipedia.org/wiki/Snake_case) for all resources and names
|
||||
- Pin Terraform version
|
||||
- Pin all providers to a major version
|
||||
|
||||
## Resources
|
||||
|
||||
- Avoid duplicating the resource type in its name
|
||||
> good: `resource "google_storage_bucket" "foo"`
|
||||
> bad: `resource "google_storage_bucket" "foo_bucket"`
|
||||
|
||||
## Variables and Outputs
|
||||
|
||||
- Remove any unused variables
|
||||
- Include a `type`
|
||||
- Include a `description` if the intent of the variable is not obvious from its name
|
||||
- Names should reflect the attribute or argument they reference in its suffix
|
||||
> good: `foo_bar_name` good: `foo_bar_id` bad: `foo_bar`
|
||||
- Use plural form in names of variables that expect a collection of items
|
||||
> multiple: `foo_bars` single: `foo_bar`
|
||||
|
||||
## Basic Project Layout
|
||||
|
||||
```
|
||||
├── main.tf # Main terraform configurations
|
||||
├── foo.tf # Terraform resources for `foo`
|
||||
├── output.tf # Output definitions
|
||||
├── provider.tf # Providers and Terraform configuration
|
||||
└── variables.tf # Variables definitions
|
||||
```
|
||||
|
||||
### `providers.tf`
|
||||
|
||||
Contains all `providers` and `terraform` blocks.
|
||||
|
||||
### `main.tf`
|
||||
|
||||
Contains Terraform `resources`, `data` and `locals` resources. On larger projects, keep generic `resource`, `data` and `locals` definitions in this file and split the rest to other files.
|
||||
|
||||
### `output.tf`
|
||||
|
||||
Contains all `output` definitions.
|
||||
|
||||
### `variables.tf`
|
||||
|
||||
Contains all `variable` definitions.
|
||||
141
doc/dev/background-information/languages/testing_go_code.md
Normal file
141
doc/dev/background-information/languages/testing_go_code.md
Normal file
@ -0,0 +1,141 @@
|
||||
# Testing Go code
|
||||
|
||||
This document contains tips for writing unit tests for Go code.
|
||||
|
||||
## Organizing code and refactoring for testability
|
||||
|
||||
If you find yourself having a difficult time testing a piece of Go code, it is likely due to one of the following factors:
|
||||
|
||||
- The code does too much
|
||||
- The code relies on global state
|
||||
- The code relies on real external services
|
||||
|
||||
In all of these cases, you will find it easier to test if you break the code into smaller units where each one can be tested independently and without global state or external behavior.
|
||||
|
||||
**If your code relies on a global or external service**, then you can refactor that piece of code to instead take a _reference_ to that service. This section runs through an example of how to refactor such code without affecting the callers. The original code relies on a global database connection, but the same issue occurs in code that refers to external APIs (GitHub, Slack, Zoekt, etc).
|
||||
|
||||
```go
|
||||
func UntestableFunction() (*SomeThing, error) {
|
||||
value, err := GlobalDbConn.DoThing()
|
||||
check(err)
|
||||
|
||||
st := // some things with value
|
||||
return st, nil
|
||||
}
|
||||
```
|
||||
|
||||
First, we break this into two parts: a public function with the same signature, and a new function that injects its dependencies explicitly.
|
||||
|
||||
```go
|
||||
func UntestableFunction() (*SomeThing, error) {
|
||||
return testableFunction(GlobalDbConn)
|
||||
}
|
||||
|
||||
func testableFunction(conn *DbConn) (*SomeThing, error) {
|
||||
value, err := conn.DoThing()
|
||||
check(err)
|
||||
|
||||
st := // some things with value
|
||||
return st, nil
|
||||
}
|
||||
```
|
||||
|
||||
If the dependency of `testableFunction` is already something that can be mocked (if the value is an interface, or the struct value can be created easily and has very little internal behavior), then most of the benefit has already been seen.
|
||||
|
||||
Otherwise, we should apply an additional step to make the dependency mockable in the tests. This requires that we extract the _interface_ of the dependency from its implementation, and take any value that conforms to that interface.
|
||||
|
||||
```go
|
||||
func UntestableFunction() (*SomeThing, error) {
|
||||
return testableFunction(GlobalDbConn)
|
||||
}
|
||||
|
||||
type IDBConn interface {
|
||||
DoThing() (*Thing, error)
|
||||
}
|
||||
|
||||
func testableFunction(conn IDBConn) (*SomeThing, error) {
|
||||
value, err := conn.DoThing()
|
||||
check(err)
|
||||
|
||||
st := // some things with value
|
||||
return st, nil
|
||||
}
|
||||
```
|
||||
|
||||
Now, the tests for this new function can create their own `IDBConn` with whatever behavior for `DoThing` that is required. See the section on [mocking](#mocks) below for more details.
|
||||
|
||||
## Testing APIs
|
||||
|
||||
External HTTP APIs should be tested with the `"httptest"` package.
|
||||
|
||||
For an example usage, see the [bundle manager client tests](https://sourcegraph.com/github.com/sourcegraph/sourcegraph@0cb60598806d68e4c4edace9ed2a801e3f8495bf/-/blob/enterprise/internal/codeintel/bundles/client/bundle_client_test.go#L13), that mock the internal bundle manager service with canned responses. Request values are asserted in the test HTTP handler itself, comparing the requested HTTP method, path, and query args against the expected values.
|
||||
|
||||
If you need to test interactions with an external HTTP API, take a look at the `"httptestutil"` package. The `NewRecorder` function can be used to create an HTTP client that records and replays HTTP requests. See [`bitbucketcloud.NewTestClient`](https://github.com/sourcegraph/sourcegraph/blob/f2e55799acad8b6b28cb3b6fd47cc55993d36dc4/internal/extsvc/bitbucketcloud/testing.go#L22-L47) for an example. Or take a look at [other usages of `httptestutil.NewRecorder`](https://sourcegraph.com/github.com/sourcegraph/sourcegraph@master/-/blob/enterprise/internal/campaigns/resolvers/main_test.go#L37:27&tab=references).
|
||||
|
||||
## Assertions
|
||||
|
||||
We use the Go's stdlib `"testing"` package to drive unit tests and assertions. Here are some tips when writing tests:
|
||||
|
||||
#### Handling unexpected errors
|
||||
|
||||
An unexpected error value in a test should be met with an immediate failure. This prevents the test from emitting spurious errors, as either the function's remaining results will be zero-values, or there was a failure to perform a specific side-effect that the remainder of the test may rely on having occurred.
|
||||
|
||||
```go
|
||||
func TestSprocket(t *testing.T) {
|
||||
value, err := NewSprocket().Sprock(context.Background())
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error sprocking: %s", err)
|
||||
}
|
||||
|
||||
// value can be asserted safely here
|
||||
}
|
||||
```
|
||||
|
||||
#### Asserting expected complex values
|
||||
|
||||
Expected values that are not simple scalars (really, anything not comparable with `==`) should use [go-cmp](https://github.com/google/go-cmp/cmp) to create a highly-readable diff string.
|
||||
|
||||
```go
|
||||
import "github.com/google/go-cmp/cmp"
|
||||
|
||||
func TestCoolPlanets(t *testing.T) {
|
||||
planets := CoolPlanets()
|
||||
|
||||
expectedPlanets := []string{
|
||||
"Neptune",
|
||||
"Uranus", // ...
|
||||
"Pluto", // we still love you
|
||||
"NOT Venus", // this thing is hot af
|
||||
}
|
||||
if diff := cmp.Diff(expectedPlanets, planets); diff != "" {
|
||||
t.Errorf("unexpected planets (-want +got):\n%s", diff)
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
*Caveat*: go-cmp uses reflection therefore all comparable fields must be exported.
|
||||
|
||||
## Mocks
|
||||
|
||||
If your code depends on a value defined as an interface (which all dependencies **should** be), you can use [efritz/go-mockgen](https://github.com/efritz/go-mockgen) to create programmable stubs that conform to the target interface. This replaces an old pattern in the code base that declared mocks defined as [globally settable functions](https://sourcegraph.com/github.com/sourcegraph/sourcegraph@b5b5fc8d885710eb559ff3d6122c9360b31fec78/-/blob/internal/vcs/git/mocks.go#L15).
|
||||
|
||||
For an example usage of generated mocks, see the [TestDefinitions](https://sourcegraph.com/github.com/sourcegraph/sourcegraph@0cb60598806d68e4c4edace9ed2a801e3f8495bf/-/blob/enterprise/internal/codeintel/resolvers/query_test.go#L16) test for the code intel resolvers. Each method defined on an interface has a default implementation that returns zero-values for all of its results and can be configured to:
|
||||
|
||||
- call a hook function on every invocation (via `SetDefaultHook`)
|
||||
- call a hook function for the _next_ invocation (via `PushHook`)
|
||||
- return canned values on every invocation (via `SetDefaultReturn`)
|
||||
- return canned values for the _next_ invocation (via `PushReturn`)
|
||||
|
||||
Using `PushHook` and `PushReturn`, you can create an ordered stack of results (e.g. return `X`, then `Y`, then `Z`, then return `W` for every subsequent invocation). Hooks also let you perform additional logic based on the input parameters, test-local state, and perform side effects on invocation.
|
||||
|
||||
Invocations can be inspected by querying the `History` of a method, which stores the parameter values and return values of each invocation of the mock. This provides a concise way to later examine invocation counts and parameter values to ensure the dependency is being called in the expected manner.
|
||||
|
||||
## Testing time
|
||||
|
||||
Testing code that interfaces with wall time is [difficult](https://github.com/sourcegraph/sourcegraph/pull/11426#issuecomment-642428217).
|
||||
|
||||
If you have code that requires use of the `"time"` package, your first action should be to refactor the code, if possible, so that the parts that deal with time and the parts that deal with the other testable logic are cleanly separable. If your function calls `time.Now`, see if it is possible to pass the current time as a parameter. If your function sleeps or requires a ticker, see if you can extract the time-irrelevant code into a function that can be tested separately.
|
||||
|
||||
If all else fails, you can make use of [efritz/glock](https://github.com/efritz/glock) to mock the behavior of the time package. This requires that the code under test uses a `glock.Clock` value rather than the time package directly (see the [section above](#organizing-code-and-refactoring-for-testability) for tips on refactoring your code). This package provides a _real_ clock implementation, which is a light wrapper over the time package, and a _mock_ clock implementation that allows the clock (and underlying tickers) to be advanced arbitrarily during the test.
|
||||
|
||||
For an example usage of a mock clock, see the [TestRetry](https://sourcegraph.com/github.com/sourcegraph/sourcegraph@0cb60598806d68e4c4edace9ed2a801e3f8495bf/-/blob/enterprise/internal/codeintel/bundles/client/retry_test.go#L13) test, which tests a retry loop with a sleeping backoff. This test decouples wall time from the clock by advancing the clock in a goroutine as far as necessary to unstick the execution of the retry loop.
|
||||
338
doc/dev/background-information/languages/typescript.md
Normal file
338
doc/dev/background-information/languages/typescript.md
Normal file
@ -0,0 +1,338 @@
|
||||
# TypeScript programming patterns
|
||||
|
||||
This is a non-exhaustive lists of programming patterns used across our TypeScript codebases.
|
||||
Since patterns are used everywhere, they are usually not documented with inline code comments.
|
||||
This document serves as the canonical place to document them.
|
||||
|
||||
This document is specifically intended for patterns that are hard to detect automatically with static analysis.
|
||||
If possible, we generally prefer to encode our best practices in ESLint configuration.
|
||||
For automatically detectable patterns, please see the documentation of our [ESLint configuration](https://github.com/sourcegraph/eslint-config#principles).
|
||||
Our code is also formatted automatically by [Prettier](https://prettier.io/) so we don't waste time bike-shedding formatting.
|
||||
|
||||
## `Subscription` bag
|
||||
|
||||
Functions, classes or React class components often register external subscriptions, usually with RxJS.
|
||||
These Subscriptions must be cleaned up when the instance of the class is no longer in use,
|
||||
but won't be automatically through garbage collection, because the external emitter holds a (transitive) reference to the class instance, not the other way around.
|
||||
|
||||
If the Subscriptions are not cleaned up, the garbage collector can not collect the class instance,
|
||||
and the subscribe callback may still run code operating on invalid state.
|
||||
In the case of React class components, it may call `setState()` after the component was unmounted,
|
||||
which causes React to throw an Error.
|
||||
|
||||
If you forgot to handle a Subscription returned by Rx `observable.subscribe()`, the ESLint rule `rxjs/no-ignored-subscription` will warn you.
|
||||
|
||||
The easiest way to solve this is to save all Subscriptions in a Subscription bag, and when the instance is disposed, unsubscribe that bag.
|
||||
[RxJS Subscriptions](https://rxjs-dev.firebaseapp.com/guide/subscription) have the nice property of being _composable_, meaning they can act as a Subscription bag.
|
||||
You can add more Subscriptions to it with the `add()` method, and unsubscribe all at once by calling `unsubscribe()`.
|
||||
It handles all the edge cases like adding a Subscription after it was already unsubscribed.
|
||||
|
||||
Subscriptions don't just accept other Subscriptions, they accept arbitrary cleanup functions to be run on unsubscription, so this pattern can also be used to clean up non-RxJS resources or listeners.
|
||||
|
||||
Example of what this pattern typically looks like in a React class component:
|
||||
|
||||
```ts
|
||||
class MyComponent extends React.Component {
|
||||
// Subscription used as a Subscription bag
|
||||
private subscriptions = new Subscription()
|
||||
|
||||
public componentDidMount(): void {
|
||||
// Register the Subscription in the Subscription bag
|
||||
this.subscriptions.add(
|
||||
someObservable.subscribe(value => {
|
||||
// This is guaranteed to never run after this.subscriptions.unsubscribe() was called
|
||||
this.setState({ value })
|
||||
})
|
||||
)
|
||||
}
|
||||
|
||||
// React lifecycle method that gets run on disposal
|
||||
public componentWillUnmount(): void {
|
||||
// Unsubscribes all Subscriptions that were made
|
||||
this.subscriptions.unsubscribe()
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
The Subscription bag pattern is usually not needed when using React function components with our [`useObservable()` family of hooks](https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/shared/src/util/useObservable.ts#L26:17), as they will handle the subscription under the hood.
|
||||
|
||||
## Making invalid states impossible through union types
|
||||
|
||||
TypeScript has a very powerful type system, including support for [union types](https://www.typescriptlang.org/docs/handbook/advanced-types.html#union-types).
|
||||
A union type `A | B | C` declares that the value can be either of type `A`, `B` or `C`.
|
||||
Often times when thinking from the perspective of the state consumer, you end up with several different state slots that need to be represented.
|
||||
For example, in a UI that fetches user data from the network, you need to show a loader when the data is loading, the contents if the result arrived, or an error if the fetch failed.
|
||||
A naive approach to model this would look like this:
|
||||
|
||||
```ts
|
||||
// BAD
|
||||
{
|
||||
isLoading: boolean
|
||||
error?: Error
|
||||
user?: User
|
||||
}
|
||||
```
|
||||
|
||||
These three properties create a matrix with a total of 2<sup>3</sup> = 8 different possible states.
|
||||
However, only 3 of these states are _valid_:
|
||||
You want to either show the loader, the error **or** the contents, never any of them at the same time.
|
||||
But by using three properties, it is possible that e.g. `isLoading` is `true`, while `user` is defined too.
|
||||
The application needs to ensure that for any change of one of the properties, the other two are reset, e.g. `isLoading` is reset to `false` as soon as the contents are loaded, as well as `errorMessage`.
|
||||
|
||||
There are two bad effects of this:
|
||||
- As the code evolves and grows more complex, these checks could easily be omitted, and the application could end up showing a loader and the contents at the same time.
|
||||
- TypeScript has no way to know that `contents` is always defined when `isLoading` is not, so you'll have to either cast in multiple places or duplicate checks. The cast could then become invalid in the future and cause errors.
|
||||
|
||||
This can be easily avoided by expressing the mutually exclusive nature of the state slots in the type system through a union type:
|
||||
|
||||
```ts
|
||||
// GOOD
|
||||
{
|
||||
userOrError?: User | Error
|
||||
}
|
||||
```
|
||||
|
||||
Here, `undefined` represents _loading_, and a defined value means either a successful fetch **or** an error.
|
||||
To find out which state is active, you can use type checking features:
|
||||
- comparing to `undefined`, `null` or a defined constant with `===`
|
||||
- using `typeof` if one of the states is a primitive type
|
||||
- using `instanceof` if one of the states is a class
|
||||
- checking a [discriminator property](https://www.typescriptlang.org/docs/handbook/advanced-types.html#discriminated-unions) like `type` or `kind` if available
|
||||
- using a [custom type guard function](https://www.typescriptlang.org/docs/handbook/advanced-types.html#type-guards-and-differentiating-types) if you need to distinguish between completely different object interfaces
|
||||
|
||||
TypeScript is then able to narrow the type correctly in the conditional branches defined by `if`/`else`, the ternary operator, or `return`:
|
||||
|
||||
```ts
|
||||
if (userOrError === undefined) {
|
||||
return <Loader />
|
||||
}
|
||||
if (userOrError instanceof Error) {
|
||||
return <ErrorAlert error={userOrError} />
|
||||
}
|
||||
return <div>Username: {userOrError.username}</div>
|
||||
```
|
||||
|
||||
We are now relying on the type system to enforce at compile time that it is impossible to have a loader and an error shown at the same time.
|
||||
|
||||
**Caveat**: If you are using the `Error` type, make sure that the exception you are saving is actually an Error with our `asError()` utility.
|
||||
Exceptions in TypeScript are of type `any`, i.e. if a function deep down exhibits the bad practice of throwing e.g. a string, it could mess up the type checking logic.
|
||||
If you accidentally return an `any` typed error without using `asError()`, the ESLint rule `no-unsafe-return` will warn you.
|
||||
|
||||
## Avoiding classes
|
||||
|
||||
In traditional OOP programming languages, classes are often used for **encapsulation** and **polymorphism**.
|
||||
In TypeScript, classes are not needed to achieve these goals.
|
||||
Encapsulation can be easily achieved with modules (i.e. non-exported module members) and closures.
|
||||
Polymorphism is available without classes thanks to duck-typed interfaces and object literals.
|
||||
|
||||
We found that when using classes as an encapsulation boundary in TypeScript, over time they often grow to violate the [Single Responsibility Principle](https://en.wikipedia.org/wiki/Single-responsibility_principle), become hard to reason about and hard to test.
|
||||
Methods often get added to the class that only access a subset of the properties of the class.
|
||||
Splitting the class up afterwards into multiple smaller classes that draw better boundaries takes a lot of effort.
|
||||
|
||||
Starting out from the beginning with **individual functions** (instead of methods) that take **just the data they need** as interface-typed parameters (instead of accessing class fields) avoids this problem.
|
||||
It makes it easy to evolve each function individually, increase or decrease their data dependencies, and split or merge them.
|
||||
Ideally these functions do not mutate the input object, but each produce a new result object instead.
|
||||
This makes it easy to compose them.
|
||||
Avoid having functions that take more data than they actually need just to return this part of the input verbatim, because it makes them harder to test and ties them to the context they are used in currently.
|
||||
This "merging" is better done by the caller.
|
||||
Instead of constructors, factory functions can be defined that create and return object literals (typed with an interface).
|
||||
These functions are conventionally named `create`+_name of interface_ (eg. [`createModelService()`](https://sourcegraph.com/github.com/sourcegraph/sourcegraph@b1ddeff4a2b94ceccda7cdf7021d5f82aa4522ed/-/blob/shared/src/api/client/services/modelService.ts#L99-167).
|
||||
|
||||
There are a few places where we do use classes, e.g. [`ExtDocuments`](https://sourcegraph.com/github.com/sourcegraph/sourcegraph@fd9eef0b5893dfb3358d2a3358d15f3e9b14ca9e/-/blob/shared/src/api/extension/api/documents.ts#L21:20).
|
||||
These are usually where mutation is unavoidable.
|
||||
For example, our extension host web worker runs in a separate thread.
|
||||
We need to sync various data between the worker and the main thread, because requesting that data on demand every time through message passing would not be performant.
|
||||
|
||||
## Converting React class components to function components
|
||||
|
||||
We have a large number of React class components in our codebase predating [React hooks](https://reactjs.org/docs/hooks-intro.html).
|
||||
We are continuously refactoring these to function components using hooks. This section provides examples of refactoring common class component patterns to into function components.
|
||||
|
||||
### `logViewEvent` calls
|
||||
|
||||
When refactoring, simply move these calls from `componentDidMount()` to a `useEffect()` callback:
|
||||
|
||||
```typescript
|
||||
useEffect(() => {
|
||||
eventLogger.logViewEvent('ApiConsole')
|
||||
}, [])
|
||||
```
|
||||
|
||||
### Fetching data in componentDidMount
|
||||
|
||||
A lot of components will fetch initial data using observables, calling `setState()` in the `subscribe()` callback. See [this example from `SiteUsageExploreSection`](https://sourcegraph.com/github.com/sourcegraph/sourcegraph@9997438a8ba2fdc54920f1f6ad22dd08d4a37215/-/blob/web/src/usageStatistics/explore/SiteUsageExploreSection.tsx?subtree=true#L32-38):
|
||||
|
||||
```typescript
|
||||
public componentDidMount(): void {
|
||||
this.subscriptions.add(
|
||||
fetchSiteUsageStatistics()
|
||||
.pipe(catchError(error => [asError(error)]))
|
||||
.subscribe(siteUsageStatisticsOrError => this.setState({ siteUsageStatisticsOrError }))
|
||||
)
|
||||
}
|
||||
```
|
||||
|
||||
In function components, this pattern can be easily replaced with `useObservable()`:
|
||||
|
||||
```typescript
|
||||
const usageStatisticsOrError = useObservable(fetchSiteUsageStatistics().pipe(catchError(error => [asError(error)])))
|
||||
```
|
||||
|
||||
If the function returning the Observable took props as parameters, you can rely on `useMemo()` to [re-create the Observable when props change](https://sourcegraph.com/github.com/sourcegraph/sourcegraph@9997438a8ba2fdc54920f1f6ad22dd08d4a37215/-/blob/web/src/enterprise/site-admin/SiteAdminLsifUploadPage.tsx?subtree=true#L25-27):
|
||||
|
||||
```typescript
|
||||
const uploadOrError = useObservable(
|
||||
useMemo(() => fetchLsifUpload({ id }).pipe(catchError(error => [asError(error)])), [id])
|
||||
)
|
||||
```
|
||||
|
||||
### `this.componentUpdates`
|
||||
|
||||
A common pattern in our class components is to declare a Subject of the component's props, named `componentUpdates`. It can be used to trigger side-effects, such as fetching data or subscribing to an Observable passed as props, only when certain props change.
|
||||
|
||||
See this example from [`<MonacoEditor/>`](https://sourcegraph.com/github.com/sourcegraph/sourcegraph@9997438a8ba2fdc54920f1f6ad22dd08d4a37215/-/blob/web/src/components/MonacoEditor.tsx?subtree=true#L129-137):
|
||||
|
||||
```typescript
|
||||
this.subscriptions.add(
|
||||
this.componentUpdates
|
||||
.pipe(
|
||||
map(({ isLightTheme }) => (isLightTheme ? SOURCEGRAPH_LIGHT : SOURCEGRAPH_DARK)),
|
||||
distinctUntilChanged()
|
||||
)
|
||||
.subscribe(theme => monaco.editor.setTheme(theme))
|
||||
)
|
||||
this.componentUpdates.next(this.props)
|
||||
```
|
||||
|
||||
This can be easily refactored with hooks by passing the relevant property as a dependency:
|
||||
|
||||
```typescript
|
||||
const theme = isLightTheme ? SOURCEGRAPH_LIGHT : SOURCEGRAPH_DARK
|
||||
useEffect(() => {
|
||||
monaco.editor.setTheme(theme)
|
||||
}, [theme])
|
||||
```
|
||||
|
||||
### Instance property subjects
|
||||
|
||||
Some class components expose subjects as instance properties, that can be used to trigger a new fetch of the data. See [`SettingsArea.refreshRequests`](https://sourcegraph.com/github.com/sourcegraph/sourcegraph@9997438a8ba2fdc54920f1f6ad22dd08d4a37215/-/blob/web/src/settings/SettingsArea.tsx?subtree=true#L73):
|
||||
|
||||
```typescript
|
||||
// Load settings.
|
||||
this.subscriptions.add(
|
||||
combineLatest([
|
||||
this.componentUpdates.pipe(
|
||||
map(props => props.subject),
|
||||
distinctUntilChanged()
|
||||
),
|
||||
this.refreshRequests.pipe(startWith<void>(undefined)),
|
||||
])
|
||||
.pipe(
|
||||
switchMap(([{ id }]) =>
|
||||
fetchSettingsCascade(id).pipe(
|
||||
switchMap(cascade =>
|
||||
this.getMergedSettingsJSONSchema(cascade).pipe(
|
||||
map(settingsJSONSchema => ({ subjects: cascade.subjects, settingsJSONSchema }))
|
||||
)
|
||||
),
|
||||
catchError(error => [asError(error)]),
|
||||
map(dataOrError => ({ dataOrError }))
|
||||
)
|
||||
)
|
||||
)
|
||||
.subscribe(
|
||||
stateUpdate => this.setState(stateUpdate),
|
||||
error => console.error(error)
|
||||
)
|
||||
)
|
||||
```
|
||||
|
||||
These can typically be refactored using `useEventObservable()`, to handle both the initial data fetch, and further refresh requests:
|
||||
|
||||
```typescript
|
||||
const [nextRefreshRequest, dataOrError] = useEventObservable(
|
||||
useCallback(
|
||||
(refreshRequests: Observable<void>) => refreshRequests.pipe(
|
||||
// Seed the pipe so that the initial fetch is triggered
|
||||
startWith<void>(undefined),
|
||||
switchMapTo(fetchSettingsCascade(props.subject.id)),
|
||||
switchMap(cascade => getMergedSettingsJSONSchema(cascade)),
|
||||
catchError(error => [asError(error) as ErrorLike])
|
||||
),
|
||||
// Pass props.subject.id as dependency so that settings are fetched again
|
||||
// when the subject passed as props changes
|
||||
[props.subject.id]
|
||||
)
|
||||
)
|
||||
```
|
||||
|
||||
### Uses of `tap()` to trigger side-effects in Observable pipes
|
||||
|
||||
Some Observable pipes use `tap()` to trigger side-effects, such as [this pipe in `<RepoRevisionContainer/>`](https://sourcegraph.com/github.com/sourcegraph/sourcegraph@9997438a8ba2fdc54920f1f6ad22dd08d4a37215/-/blob/web/src/repo/RepoRevisionContainer.tsx?subtree=true#L127-167) with its multiple calls to `onResolvedRevisionOrError`:
|
||||
|
||||
```typescript
|
||||
// Fetch repository revision.
|
||||
this.subscriptions.add(
|
||||
repoRevisionChanges
|
||||
.pipe(
|
||||
// Reset resolved revision / error state
|
||||
tap(() => this.props.onResolvedRevisionOrError(undefined)),
|
||||
switchMap(({ repoName, revision }) =>
|
||||
defer(() => resolveRevision({ repoName, revision })).pipe(
|
||||
// On a CloneInProgress error, retry after 1s
|
||||
retryWhen(errors =>
|
||||
errors.pipe(
|
||||
tap(error => {
|
||||
if (isCloneInProgressErrorLike(error)) {
|
||||
// Display cloning screen to the user and retry
|
||||
this.props.onResolvedRevisionOrError(error)
|
||||
return
|
||||
}
|
||||
// Display error to the user and do not retry
|
||||
throw error
|
||||
}),
|
||||
delay(1000)
|
||||
)
|
||||
),
|
||||
// Save any error in the sate to display to the user
|
||||
catchError(error => {
|
||||
this.props.onResolvedRevisionOrError(error)
|
||||
return []
|
||||
})
|
||||
)
|
||||
)
|
||||
)
|
||||
.subscribe(
|
||||
resolvedRevision => {
|
||||
this.props.onResolvedRevisionOrError(resolvedRevision)
|
||||
},
|
||||
error => {
|
||||
// Should never be reached because errors are caught above
|
||||
console.error(error)
|
||||
}
|
||||
)
|
||||
)
|
||||
```
|
||||
|
||||
This is easily solved by decoupling data production and side-effects:
|
||||
|
||||
```typescript
|
||||
const resolvedRevisionOrError = useObservable(
|
||||
useMemo(
|
||||
() => resolveRevision({ repoName: props.repo.name, revision: props.revision}).pipe(
|
||||
catchError(error => {
|
||||
if (isCloneInProgressErrorLike(error)) {
|
||||
return [error]
|
||||
}
|
||||
throw error
|
||||
}),
|
||||
repeatUntil(value => !isCloneInProgressErrorLike(value), { delay: 1000 }),
|
||||
catchError(error => [asError(error)])
|
||||
),
|
||||
[props.repo.name, props.revision]
|
||||
)
|
||||
)
|
||||
useEffect(() => {
|
||||
props.onResolvedRevisionOrError(resolvedRevisionOrError)
|
||||
}, [resolvedRevisionOrError, props.onResolvedRevisionOrError])
|
||||
```
|
||||
29
doc/dev/background-information/security_patterns.md
Normal file
29
doc/dev/background-information/security_patterns.md
Normal file
@ -0,0 +1,29 @@
|
||||
# Security patterns
|
||||
|
||||
## Authorization
|
||||
|
||||
Authorization is the gatekeeper for securing private resources by preventing unauthorized access. Examples of private resources are:
|
||||
|
||||
- Contents of a private repository
|
||||
- Endpoints that can only be called by certain users (e.g. site admins)
|
||||
- User settings or organization settings
|
||||
|
||||
Current forms of authorization in Sourcegraph include:
|
||||
|
||||
- Site admin roles
|
||||
- Organization memberships
|
||||
- Campaigns permissions
|
||||
- Repository permissions
|
||||
- Same-user validation
|
||||
|
||||
As a standard practice, users who do not have access to a given private resource should not be aware of the existence of that private resource. When only part of a resource is restricted by authorization limitations, it is reasonable to prompt the user that some resources are not available due to insufficient permissions. However, we must not provide anything that indicates what those restricted resources might be.
|
||||
|
||||
### Enforce authorization
|
||||
|
||||
In Sourcegraph, there are two places to enforce authorization, both equally important:
|
||||
|
||||
- At the GraphQL layer:
|
||||
- Some endpoints are restricted to certain users (e.g. site admins or the same user).
|
||||
- Be aware that any backend failure has potential to indicate unauthorized information about private resources. Therefore, halt the process as soon as we identify an unauthorized request, and behave as if the resource does not exist at all.
|
||||
- At the database layer:
|
||||
- The database is our source of truth for authorization, especially for repository and campaigns permissions. Enforcing authorization at this layer is absolutely necessary.
|
||||
17
doc/dev/background-information/testing-pyramid.svg
Normal file
17
doc/dev/background-information/testing-pyramid.svg
Normal file
File diff suppressed because one or more lines are too long
|
After Width: | Height: | Size: 28 KiB |
@ -1,6 +1,6 @@
|
||||
# Testing
|
||||
|
||||
_This documentation is specifically for the tests in the [sourcegraph/sourcegraph](https://github.com/sourcegraph/sourcegraph) repository. For our general testing principles, please refer to our [testing guide in the handbook](https://about.sourcegraph.com/handbook/engineering/testing)._
|
||||
_This documentation is specifically for the tests in the [sourcegraph/sourcegraph](https://github.com/sourcegraph/sourcegraph) repository. For our general testing principles, please see "[Testing Principles](testing_principles.md)".
|
||||
|
||||
## Backend tests
|
||||
|
||||
|
||||
123
doc/dev/background-information/testing_principles.md
Normal file
123
doc/dev/background-information/testing_principles.md
Normal file
@ -0,0 +1,123 @@
|
||||
# Testing principles
|
||||
|
||||
This file documents how we test code at Sourcegraph.
|
||||
|
||||
_For practical information on how we test things in the [sourcegraph/sourcegraph](https://github.com/sourcegraph/sourcegraph) repository, see "[Testing](testing.md)"._
|
||||
|
||||
## Philosophy
|
||||
|
||||
We rely on automated testing to ensure the quality of our product.
|
||||
|
||||
Any addition or change to our codebase should be covered by an appropriate amount of automated tests to ensure that:
|
||||
|
||||
1. Our product and code works as intended when we ship it to customers.
|
||||
1. Our product and code doesn't accidentally break as we make changes over time.
|
||||
|
||||
A good automated test suite increases the velocity of our team because it allows engineers to confidently edit and refactor code, especially code authored by someone else.
|
||||
|
||||
Engineers should budget an appropriate amount of time for writing tests when making iteration plans.
|
||||
|
||||
## Flaky tests
|
||||
|
||||
A *flaky* test is defined as a test that is unreliable or non-deterministic, meaning that it doesn't consistently produce the same pass/fail result.
|
||||
|
||||
Typical reasons why a test may be flaky:
|
||||
|
||||
- Race conditions or timing issues
|
||||
- Caching or inconsistent state between tests
|
||||
- Unreliable test infrastructure (such as CI)
|
||||
- Reliance on third-party services that are inconsistent
|
||||
|
||||
We do not tolerate flaky tests of any kind. Any engineer that sees a flaky test should immediately:
|
||||
|
||||
1. Open a PR to disable the flaky test.
|
||||
1. Open an issue to re-enable the flaky test, assign it to the most likely owner, and add it to the current release milestone.
|
||||
|
||||
If the build or test infrastructure itself is flaky, then [open an issue](https://github.com/sourcegraph/sourcegraph/issues/new?labels=team/distribution) and notify the [distribution team](https://about.sourcegraph.com/handbook/engineering/distribution#contact).
|
||||
|
||||
Why are flaky tests undesirable? Because these tests stop being an informative signal that the engineering team can rely on, and if we keep them around then we eventually train ourselves to ignore them and become blind to their results. This can hide real problems under the cover of flakiness.
|
||||
|
||||
## Testing pyramid
|
||||
|
||||
|
||||
|
||||
### Unit tests
|
||||
|
||||
Unit tests test individual functions in our codebase and are the most desirable kind of test to write.
|
||||
|
||||
Benefits:
|
||||
|
||||
- They are usually very fast to execute because slow operations can be mocked.
|
||||
- They are the easiest tests to write, debug, and maintain because the code under test is small.
|
||||
- They only need to run on changes that touch code which could make the test fail, which makes CI faster and minimizes the impact of any [flakiness](#flaky-tests).
|
||||
|
||||
Tradeoffs:
|
||||
|
||||
- They don't verify our systems are wired up correctly end-to-end.
|
||||
|
||||
### Integration tests
|
||||
|
||||
Integration tests test the behavior of a subset of our entire system to ensure that subset of our system is wired up correctly.
|
||||
|
||||
Benefits:
|
||||
|
||||
- To the extent that fewer systems are under test compared to e2e tests, they are faster to run, easier to debug, have clearer ownership, and less vulnerable to [flakiness](#flaky-tests).
|
||||
- They only need to run on changes that touch code which could make the test fail, which makes CI faster and minimizes the impact of any [flakiness](#flaky-tests).
|
||||
|
||||
Tradeoffs:
|
||||
|
||||
- They don't verify our systems are wired up correctly end-to-end.
|
||||
- They are not as easy to write as unit tests.
|
||||
|
||||
Examples:
|
||||
|
||||
- Tests that call our search API to test the behavior of our entire search system.
|
||||
- Tests that validate UI behavior in the browser while mocking out all network requests so no backend is required.
|
||||
|
||||
#### Running integration tests
|
||||
|
||||
Integration tests are run everytime a branch is merged into main, but it is possible to run them manually:
|
||||
|
||||
- Create a branch with the `master-dry-run/` prefix, example: `master-dry-run/my-feature`
|
||||
- Push it on Github
|
||||
- Look for that branch on [Buildkite](https://buildkite.com/sourcegraph/sourcegraph)
|
||||
|
||||
### End-to-end tests (e2e)
|
||||
|
||||
E2e tests test our entire product from the perspective of a user. We try to use them sparingly. Instead, we prefer to get as much confidence as possible from our [unit tests](#unit-tests) and [integration tests](#integration-tests).
|
||||
|
||||
Benefits:
|
||||
|
||||
- They verify our systems are wired up correctly end-to-end.
|
||||
|
||||
Tradeoffs:
|
||||
|
||||
- They are typically the slowest tests to execute because we have to build and run our entire product.
|
||||
- They are the hardest tests to debug because failures can be caused by a defect anywhere in our system. This can also make ownership of failures unclear.
|
||||
- They are the most vulnerable to [flakiness](#flaky-tests) because there are a lot of moving parts.
|
||||
|
||||
Examples:
|
||||
|
||||
- Run our Sourcegraph Docker image and verify that site admins can complete the registration flow.
|
||||
- Run our Sourcegraph Docker image and verify that users can sign in and perform a search.
|
||||
|
||||
### Visual testing
|
||||
|
||||
We use [Percy](https://percy.io/) to detect visual changes in Sourcegraph features. You may need permissions to update screenshots if your feature introduces visual changes. Post a message in #dev-chat that you need access to Percy, and someone will add you to our organization (you will also receive an invitation via e-mail). _Do not_ create an account on Percy before receiving the invitation: as Percy has been acquired by BrowserStack, if you create an account outside of the invitation flow, you will end up with a BrowserStack account that cannot be added to a Percy organization.
|
||||
|
||||
Once you've been invited to the Sourcegraph organization and created a Percy account, you should then link it to your GitHub account.
|
||||
|
||||
## Ownership
|
||||
|
||||
- [Distribution team](https://about.sourcegraph.com/handbook/engineering/distribution) owns build and test infrastructure.
|
||||
- [Web team](https://about.sourcegraph.com/handbook/engineering/web) owns any tests that are driven through the browser.
|
||||
|
||||
## Conventions
|
||||
|
||||
- **Naming tests in Go code.** We strive to follow the same naming convention for Go test functions as described for [naming example functions in the Go testing package](https://golang.org/pkg/testing/#hdr-Examples).
|
||||
|
||||
## See also
|
||||
|
||||
- [Documentation for running tests in sourcegraph/sourcegraph](testing.md)
|
||||
- [Go-specific testing guide](https://about.sourcegraph.com/handbook/engineering/languages/testing_go_code)
|
||||
- [Web-specific testing guide](https://about.sourcegraph.com/handbook/engineering/web/testing)
|
||||
9
doc/dev/how-to/adding_buildkite_secrets.md
Normal file
9
doc/dev/how-to/adding_buildkite_secrets.md
Normal file
@ -0,0 +1,9 @@
|
||||
# Adding or changing Buildkite secrets
|
||||
|
||||
This page outlines the process for adding or changing secrets available on our Buildkite agents.
|
||||
|
||||
## Adding secrets to the Buildkite agent environment
|
||||
|
||||
- Add the secret to the [Buildkite agent deployment](https://github.com/sourcegraph/infrastructure/blob/main/buildkite/kubernetes/buildkite-agent/buildkite-agent.Deployment.yaml)
|
||||
- Commit the secret to the master branch
|
||||
- Follow the [instructions](https://github.com/sourcegraph/infrastructure/tree/main/buildkite#deploying-kubernetes) to deploy the updated configuration to the cluster
|
||||
18
doc/dev/how-to/ignoring_editor_config_files.md
Normal file
18
doc/dev/how-to/ignoring_editor_config_files.md
Normal file
@ -0,0 +1,18 @@
|
||||
# Ignoring editor config files in Git
|
||||
|
||||
Many editors create config files when working in a project directory. For example, the `.idea/` directory that IntelliJ creates.
|
||||
|
||||
You can ignore these in Git using one of two options:
|
||||
|
||||
- Globally, in all Git repositories via `~/.gitignore`
|
||||
- Per-repository, via `.git/info/exclude` (which is just like `.gitignore` but not committed to the repository)
|
||||
|
||||
## Why not commit .gitignore editor config files?
|
||||
|
||||
There are many editors that produce repository config files, so if we commit them to `.gitignore` questions arise:
|
||||
|
||||
- Do we do this for every editor people at Sourcegraph use?
|
||||
- How do we keep this `.gitignore` in sync with all the other repositories?
|
||||
- When creating a new repository, what should go in `.gitignore`?
|
||||
|
||||
Instead of maintaining all that, we keep things simple and do not commit editor configs, nor their exclusions, to `.gitignore` in the repository and configure it as part of our development environments instead.
|
||||
@ -4,6 +4,7 @@
|
||||
|
||||
- [How to debug live code](debug_live_code.md)
|
||||
- [Set up local development with Zoekt and Sourcegraph](zoekt_local_dev.md)
|
||||
- [Ignoring editor config files in Git](ignoring_editor_config_files.md)
|
||||
|
||||
## New features
|
||||
|
||||
@ -29,11 +30,14 @@
|
||||
- [How to add monitoring](add_monitoring.md)
|
||||
- [Set up local Sourcegraph monitoring development](monitoring_local_dev.md)
|
||||
|
||||
## Testing Sourcegraph
|
||||
## Testing Sourcegraph & CI
|
||||
|
||||
- [How to run tests](../background-information/testing.md)
|
||||
- [Testing Principles](../background-information/testing_principles.md)
|
||||
- [Continuous Integration](../background-information/continuous_integration.md)
|
||||
- [Configure a test instance of Phabricator and Gitolite](configure_phabricator_gitolite.md)
|
||||
- [Test a Phabricator and Gitolite instance](test_phabricator.md)
|
||||
- [Adding or changing Buildkite secrets](adding_buildkite_secrets.md)
|
||||
|
||||
## Windows support
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user