-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce BUILD.md #137
Introduce BUILD.md #137
Conversation
|
||
- [`proxy`](proxy): High-performance data plane, injected as a sidecar with | ||
every service. | ||
- [`tower-grpc`](tower-grpc): A client and server gRPC implementation based on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we are hoping to stop vendoring these library dependencies and depend on them from their Git repos --- see #119. Hopefully this will happen soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some notes on the proxy parts.
BUILD.md
Outdated
|
||
## Data Plane (Rust) | ||
|
||
- [`proxy`](proxy): High-performance data plane, injected as a sidecar with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to break down some of the modules inside the proxy project eventually, like you've done for the controller?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea. I've filed #148 to track this, I think the Rust folks are better equipped to document this.
BUILD.md
Outdated
## Rust | ||
|
||
```bash | ||
cargo check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is essentially the same as running cargo build
without producing a binary; it just invokes the analysis portion of the compiler and skips codegen. useful if you want to know if your changes will compile, but not sure if it's really "testing"...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds like cargo test
is a superset. i'm going to leave this here for now so that it matches .travis.yml
. i've filed #149 to follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I'm coming to this doc, I want to learn one of a few things:
- How do I build/test the Go stuff?
- How do I build/test Web stuff?
- How do I build/test Rust stuff?
- How do I build/test release artifacts?
It answers most of these things, but I think it would be easier to get at this information if the doc was organized around these core tasks instead of around the individual tools.
I think we can leave out things like the description of all of the components. We'll add components, etc, and I don't think we should have to update this document every time that happens. For that sort of thing, I'd prefer that we rely on generated docs from the code itself so it's much harder for it to be wrong.
BUILD.md
Outdated
- [Rust](https://www.rust-lang.org) | ||
- [Minikube](https://github.com/kubernetes/minikube) | ||
- [Docker](https://www.docker.com/) | ||
- [Yarn](https://yarnpkg.com) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to reference each of these dependencies in the specific sections, with links/descriptions to our recommended install instructions for each.
Having this at the top makes me think i have to go install all of these things first and foremost, which isn't true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I've re-org'd the doc around Go/Web/Rust/Release, with each section documenting its own dependencies, build, and test commands.
BUILD.md
Outdated
|
||
<details> | ||
<summary></summary> | ||
build_architecture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do others think that this adds clarity? To me this just says "it's complex." I find the scripts themselves to be a bit easier approach than a big dependency graph... If they're not approachable, perhaps they could use some better documentation internally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I found the labels on here to be too small to read without straining my eyes. I think some form of graph could be useful, but all I really got out of this was "we have too many different shell scripts"...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While not specifically necessary to build conduit, I found this graph invaluable when debugging build issues or making changes in #117 and #123. Without this graph in documentation, I'd have to reconstitute it in my head each time I context switch into working on the build.
@hawkw Agree the labels are a bit small. The image is clickable to expand. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Really happy to see all of this documented and in the same file. I left a smattering of comments, but most are TIOLI -- just some musings about how to make the file a bit more navigable.
@@ -0,0 +1,472 @@ | |||
:balloon: Welcome to the Conduit development guide! :wave: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BUILD.md
Outdated
|
||
# Repo layout | ||
|
||
Conduit is primarily writtern in Go and Rust (and JavaScript!). At its core |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo here: writtern
=> written
.
And I'd actually include React as a first-class language.
Conduit is primarily written in Go, Rust and React. At its core
BUILD.md
Outdated
|
||
This document will help you build Conduit from source. Depending on the | ||
components you are interested in building, these instructions assume you have | ||
the following tools already setup: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a pretty long file, it might be nice to have a TOC at the top of the document that links to each of the component sections. Something like:
* [Components](#components)
* [proxy](#proxy)
* [proxy-api](#proxy-api)
* [public-api](#public-api)
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there are tools for auto-generating a Markdown TOC...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this file might be trying to do too much in a single place. I see three use cases for the write-up here:
- I want to build Conduit from Master, either because I want to test the HEAD or because I've made a small change and want to make sure it works. Basically, all I need to know is what commands to run to get master deployed somewhere.
- I want to contribute to Conduit and need to understand how the code is structured, what development practices to follow, and what each directory contains. I need to know about the modules, what is allowed or not, and how do I run a developer build and tests.
- I want to understand Conduit's architecture, it's major components and how they relate to each other and to my application. I need to understand how the CLIs talks to the controller and how the controller talks to proxies.
I would suggest that we need a specific doc per item, something like BUILD.md
, DEVELOP.md
, and ARCHITECTURE.md
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree this is a lot of info in one doc. I did consider breaking out an ARCHITECTURE.md
, particularly for the dependency graphs. Having just consolidated info from several README's, I'd like to leave this info in a single doc for now. As specific sections increase in complexity, let's break them out at that point.
"cli" -> "public-api"; | ||
"web" -> "public-api"; | ||
|
||
"destination" -> "kubernetes"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "tap" and "destination" services also have a dependency on "kubernetes".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I'm concerned about maintaining this level of detailed information up-to-date while the project is in flux. Is this information needed to understand how to build the project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree there is risk this will get stale quickly. I find this graph provides a ton of context to someone new to the project (it helped me to write this doc). Similar to the build graph, it may not be strictly required to build Conduit, but can help the moment a developer encounters something unexpected.
Also agree generating this from code would be ideal, but until we have that, I'd prefer to have this documented somewhere.
BUILD.md
Outdated
dep ensure && dep prune | ||
|
||
# build and install conduit cli locally | ||
go build -o $GOPATH/bin/conduit ./cli |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal preference, but I don't usually install the local cli into my global bin dir. Instead, I just run the cli locally with, for example:
bin/go-run cli version
BUILD.md
Outdated
conduit status | ||
|
||
# build all docker images, using minikube as our docker repo | ||
DOCKER_FORCE_BUILD=1 DOCKER_TRACE=1 bin/mkube bin/docker-build latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove DOCKER_FORCE_BUILD
here. It slows things down a lot, and shouldn't be required if the build system is working as expected. You might just want to call it out separately as a debugging tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DOCKER_FORCE_BUILD
may not be doing what we think it's doing, created #141 to track this.
BUILD.md
Outdated
|
||
# Go run | ||
|
||
Our Go build instructions use a [`bin/go-run`](bin/go-run) script in lieu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might make more sense near the top of the file, since it's used in some of the sections above this section. Maybe, in terms of the layout of this file, we could do something like:
- Languages & Frameworks
- Rust (include info about
cargo
) - Go (include info about
dep
andgo-run
) - React (include info about
webpack
andyarn
)
- Rust (include info about
- Components (for each: short description, where it lives in the repo, how to run it from the command line)
- Data plane
- proxy
- proxy-init
- ...
- Control plane
- proxy-api
- public-api
- ...
- Interface layer
- cli
- web
- Data plane
- Development environments (local dev covered in previous section)
- minikube
- docker-compose
- Testing
- Rust
- Go
- React
- Dependencies
- Protobuf
- Docker
Or... something like that. I think they layout you came up with is a good way to represent all of this info too. It's just a lot of info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BUILD.md
Outdated
|
||
"tap" -> "proxy"; | ||
|
||
"telemetry" -> "prometheus"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically every go service has a dependency on prometheus, since prometheus scrapes metrics from each service and those metrics are reported on the Service Mesh overview page in the web UI. But as far as the data plane => control plane interaction goes, only telemetry writes to prometheus, which is what you have here. I'm a bit torn on whether or not it would be worth reflecting the other dependencies. It would probably complicate the graph, so fine as is, but just wanted to mention it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⭐️ Great, thanks for updating! This is an enormous improvement over the current setup.
BUILD.md
Outdated
# Table of contents | ||
|
||
- [Repo Layout](#repo-layout) | ||
- [Control Plane (Go)](#control-plane-go) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this link text to "Control Plane (Go/React)" to match the heading? And I think the anchor should be "#control-plane-goreact".
BUILD.md
Outdated
"tap" -> "kubernetes"; | ||
"tap" -> "proxy"; | ||
|
||
"telemetry" -> "prometheus"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're still mssing "telemetry" -> "kubernetes"
in this list.
Our build instructions were scattered across a few README's. This consolidates all instructions relevant to Conduit development into a single BUILD.md. Fixes #134 Signed-off-by: Andrew Seigner <andrew@sig.gy>
Our build instructions were scattered across a few README's. This consolidates all instructions relevant to Conduit development into a single BUILD.md. Fixes linkerd#134 Signed-off-by: Andrew Seigner <andrew@sig.gy>
The `ctx` module is no longer used. It can safely be deleted.
Our build instructions were scattered across a few README's.
This consolidates all instructions relevant to Conduit development into
a single BUILD.md.
Fixes #134
Signed-off-by: Andrew Seigner andrew@sig.gy