Update status of Nix workflows.
Using this as a strategic guide on what to prioritize.
Details
Details
- Reviewers
ashoat varun • abosh - Commits
- rCOMMffa0f00038d3: [Docs] Update nix dev env status
https://github.com/CommE2E/comm/blob/jonringer/update-nix-status/docs/nix_dev_env.md
looks right given current tasks.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- jonringer/update-nix-status
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
- We shouldn't go green on something that has a "mixed" workflow until we have the instructions for setting it up settled out
- We still have steps that are necessary for C++ services that aren't listed in the Nix install instructions (eg. install Docker), right? If so we should have instructions for that too
High-level feedback – we shouldn't distinguish "mixed"... rather we can consider all workflows wil almost certainly require manual install steps, and we should document those manual install steps. Otherwise people won't know how to set-up the "Nix" workflow
Comment Actions
Not too sure about the Nix side of this (looks like @ashoat already gave some high-level feedback which should take precedence over my feedback) but there are some things that can be cleaned up on the Markdown level.
| docs/nix_dev_env.md | ||
|---|---|---|
| 11 ↗ | (On Diff #15882) | Not sure we need the Nix here. If we are truly switching to Nix and that's the way developers will be able to produce a dev environment, we can just rename to Supported workflow progress since the Nix is implied. |
| 11–25 ↗ | (On Diff #15882) | Put all the edits in one big edit since it's easier to read that way. I find Markdown easier to edit with a mirrored view of what it actually looks like, anyways:  |
| docs/nix_dev_env.md | ||
|---|---|---|
| 11–25 ↗ | (On Diff #15882) | *Meant to write Workflow, not Workflows as the name of the first column. |
Comment Actions
We shouldn't go green on something that has a "mixed" workflow until we have the instructions for setting it up settled out
I'm just noting what is supported in a technical sense. The documentation work will require quite a bit more work https://linear.app/comm/issue/ENG-1650. I'm fine with just having a red X until it's documented.
(eg. install Docker), right? If so we should have instructions for that too
The eventual goal is to remove docker as a requirement. I've removed the docker requirement for building and unit tests, but the services still expect certain files to exist in order to run. RabbitMQ and localstack can be enabled without docker in a similar fashion to redis and mariadb.
High-level feedback – we shouldn't distinguish "mixed"... rather we can consider all workflows wil almost certainly require manual install steps, and we should document those manual install steps. Otherwise people won't know how to set-up the "Nix" workflow
Reworded comments to make nix installation apparent as a required step.
| docs/nix_dev_env.md | ||
|---|---|---|
| 11 | Not sure if we should capitalize each letter here. We don't usually do that in dev_environment.md unless it's the name of a technology (Node Version Manager). Examples include: "Codegen for gRPC" (lowercase "for"), "Git repo" (lowercase "repo"), "Flow typechecker", "Running keyserver", etc. Also it looks better in a Markdown editor, in my opinion. But it can be hard to see the difference in just a text file. | |
| 22–24 | Don't think the colons need to be there because of the bold. See my attached screenshot, it looks better without the colons than with (assuming you're using a Markdown viewer to view the Markdown when editing). | |
| 24 | Does the "installing" still need to be there if it's already after the "e.g."? | |
Comment Actions
I'm fine with just having a red X until it's documented.
Okay cool, let's do that.
The eventual goal is to remove docker as a requirement
As I've indicated before I think Docker will always be a requirement given that a Docker config is a keyserver deliverable to users. Are you suggesting revisiting that or is there something I'm missing?
| docs/nix_dev_env.md | ||
|---|---|---|
| 13 ↗ | (On Diff #15887) | I don't think we should have these columns. Can we go back to a single column? I don't think developers need to know all of this, I think it's rather confusing without context, and I think we should focus on the basic question the dev wants to know... can they follow the instructions on this page to enable this workflow? |
| 13 ↗ | (On Diff #15887) | What am I seeing on the left? I think you clobbered the diff... something seems to have gone wrong with your Git / Phabricator workflow |
| 15 ↗ | (On Diff #15887) | We need Docker for keyserver, but it's not necessary for most development (just production deploy)... not sure if that counts as "requires additional undocumented steps". We can maybe leave it out for now but it should be mentioned in the docs when we get there. We should probably prioritize the docs soon |
Comment Actions
As I've indicated before I think Docker will always be a requirement given that a Docker config is a keyserver deliverable to users.
Yep that makes sense. Just concerned with developer workflow for the immediate future.
Are you suggesting revisiting that or is there something I'm missing?
If you want, but docker will probably be the easiest. I'm just concerned about the local developer workflow. Docker adds a lot of complexity, like having to do base-image dances to make CI happy. As a deliverable, I think docker makes a lot of sense, but I think it creates more problems than it saves for development.
| docs/nix_dev_env.md | ||
|---|---|---|
| 13 ↗ | (On Diff #15887) | Fair, this is mostly for my own organization. Does notion support tables? I guess it does, moved table to here: https://www.notion.so/commapp/Nix-Dev-Env-Status-170d0600d2a447fa840eb720fb11e951 I'll reduce the table to a GO / NO GO column. |
| 13 ↗ | (On Diff #15887) | You're seeing 80 character wrapping look awful. I'm reducing the table to just 2 columns, as this table will go into notion; which should also solve the wrapping issue in this diff. https://www.notion.so/commapp/Nix-Dev-Env-Status-170d0600d2a447fa840eb720fb11e951 |
| 15 ↗ | (On Diff #15887) |
Correct, just concerned with developer workflow for now. |
| docs/nix_dev_env.md | ||
|---|---|---|
| 13 ↗ | (On Diff #15887) | Ooops, this was me. Looks like I messed up syncing the Modified diff comment on another computer. |
| docs/nix_dev_env.md | ||
|---|---|---|
| 13 ↗ | (On Diff #15895) | Not sure why you changed the header here. You changed "macOS" (correct) to "MacOS" (incorrect) |
| docs/nix_dev_env.md | ||
|---|---|---|
| 17–19 ↗ | (On Diff #15895) | I think you'll need a space in here if you actually want it to show up bolded. At least, that's what my Markdown editor shows: Without space:  With space:  If it looks weird, you can change it to footnotes, or something, but that's outside the scope of this diff. |
| docs/nix_dev_env.md | ||
|---|---|---|
| 13 ↗ | (On Diff #15937) | Still think this should be reverted. If you insist on keeping it let's at least keep the capitalization consistent... s/Supported/supported/ |
| docs/nix_dev_env.md | ||
|---|---|---|
| 13 ↗ | (On Diff #15937) | Hmm, looked it up, guess this most "accepted" way of capitalization for publications. Guess I've either never noticed |