Initial documentation on setting up your own self-hosted keyserver through aws
Details
Was able to, from a clean aws account, deploy a fresh running keyserver following only directions from the docs
Diff Detail
- Repository
- rCOMM Comm
- Branch
- optional_migration_takedowns
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
docs/keyserver_aws_self_host.md | ||
---|---|---|
1 ↗ | (On Diff #43443) | Hmmm... shouldn't we have a header here, eg. "Self-hosting a keyserver on AWS"? If we add that, I think all the headers should go down a level, eg. ## -> ### |
15–17 ↗ | (On Diff #43443) | We have several prerequisites for running the scripts and Terraform logic that will help you manage your keyserver on AWS. These prerequisites are all packaged together in our provided Nix development shell. |
38 ↗ | (On Diff #43443) | It's weird that this is a subsection of "Create development shell". Can you rethink the "Table of Contents" here? |
62 ↗ | (On Diff #43443) | |
64–66 ↗ | (On Diff #43443) | Not sure we need these lines |
68 ↗ | (On Diff #43443) | What does "entered past" mean? |
87 ↗ | (On Diff #43443) | Weird that the next line has a : but this line doesn't |
90 ↗ | (On Diff #43443) | Nit: I'd drop the trailing slash |
96 ↗ | (On Diff #43443) | I don't see what touch is doing here. I can't think of a text editor that cares whether a file exists prior to being edited. Why not just use vim, and suggest the user substitute with their desired text editor? You could also use $EDITOR |
113 ↗ | (On Diff #43443) | What's this mean? What it explained earlier? |
117 ↗ | (On Diff #43443) | |
119 ↗ | (On Diff #43443) | |
121 ↗ | (On Diff #43443) | Don't think we need this line |
121–123 ↗ | (On Diff #43443) | |
127 ↗ | (On Diff #43443) | It's optional, right? |
129 ↗ | (On Diff #43443) | |
135 ↗ | (On Diff #43443) | I think we can remove this line |
137 ↗ | (On Diff #43443) | |
139 ↗ | (On Diff #43443) | Can we separate out the line of code? |
143 ↗ | (On Diff #43443) | Think "above specified" is obvious |
149 ↗ | (On Diff #43443) | |
154–162 ↗ | (On Diff #43443) | Don't we need more configs? How about all the license keys? I believe that I documented somewhere on Linear the list of params that I thought we needed on keyserver vs. web vs. landing... can you dig up that Linear comment, and try to make sure you cover everything the keyserver needs? |
165 ↗ | (On Diff #43443) | Not all variables need to be modified, right? Eg. the CORS config |
176 ↗ | (On Diff #43443) | It's weird to use inline code formatting for a separate line of code. Why not use the same three-backticks approach you use directly above? |
178 ↗ | (On Diff #43443) | |
183 ↗ | (On Diff #43443) | |
189 ↗ | (On Diff #43443) | I thought ALIAS was non-standard. Don't some domain registrars frame this as a CNAME at the root? Do we need to explain the requirement of support for the non-standard ALIAS / root CNAME record earlier? |
docs/keyserver_aws_self_host.md | ||
---|---|---|
38 ↗ | (On Diff #43443) | This might have been a misunderestanding. Create a development shell is just a comment. Removing it because The Nix development shell can be run with: is right above and the comment isn't necessary. |
68 ↗ | (On Diff #43443) | Better word here wouldn've been skipped. Including in next rebase |
87 ↗ | (On Diff #43443) | Going to keep this consistent by always include : |
96 ↗ | (On Diff #43443) | Good point. This is specific to my habits. Going to just replace with suggesting they use their own desired text editor |
113 ↗ | (On Diff #43443) | This rightfully could be confusing. Original instructions didn't have users set a region if they were using export to use their aws credentials. I'm going to change prior instructions to just require setting AWS_REGION by default. |
127 ↗ | (On Diff #43443) | Currently not optional. terraform apply needs access to MariaDB to initialize the Comm database |
189 ↗ | (On Diff #43443) | ALIAS is non-standard. I'll include a comment above about the need for ALIAS and root CNAME records with a recommendation to configure dns rules through route53 if their domain registrar does not support these records |
docs/keyserver_aws_self_host.md | ||
---|---|---|
1 | Let's make "keyserver" not capitalized. We seem to be inconsistent about this in top-level headings, but in general we don't capitalize words in headings unless they're the first word, or they're part of a proper noun | |
3 | This is the only second-level header. As requested in my last review, please re-think the table of contents | |
65 | ||
112 | Why doesn't this paragraph have a line break above it, like all the other paragraphs? | |
116 | "Allows" is a really weird word choice for something that is required | |
145–153 | Don't we need more configs? How about all the license keys? I believe that I documented somewhere on Linear the list of params that I thought we needed on keyserver vs. web vs. landing... can you dig up that Linear comment, and try to make sure you cover everything the keyserver needs? | |
156 | Not all variables need to be modified, right? Eg. the CORS config |
Getting close, but still some comments here
docs/keyserver_aws_self_host.md | ||
---|---|---|
111 ↗ | (On Diff #43950) | Can you go through the doc and replace all the places where you used a single-quote character for an apostrophe with a proper apostrophe character? Like this: ’ |
147–148 ↗ | (On Diff #43950) |
|
156 ↗ | (On Diff #43950) | Should we skip the bullet point here? Seems unnecessary and inconsistent with the following sections |
162 ↗ | (On Diff #43950) | |
168–172 ↗ | (On Diff #43950) | I looked around the keyserver code and I actually don't think this is used there... only for web / landing. Let's remove it |
174 ↗ | (On Diff #43950) | |
180 ↗ | (On Diff #43950) | |
188 ↗ | (On Diff #43950) | |
192 ↗ | (On Diff #43950) | |
206 ↗ | (On Diff #43950) | |
211 ↗ | (On Diff #43950) | The spacing is different here from the other ones. Can you make the whitespace consistent across all the example configs that include JSON? |
229 ↗ | (On Diff #43950) | Technically the user can pick whatever they want for this, right? Should we move it next to the other database credentials? |
234 ↗ | (On Diff #43950) | Terraform is a proper noun. Please make sure you capitalize all proper nouns where appropriate |
Please address inline comments before landing, and do a final sweep for apostrophes
docs/keyserver_aws_self_host.md | ||
---|---|---|
171 ↗ | (On Diff #44002) | Replace apostrophe |
177 ↗ | (On Diff #44002) | Replace apostrophe |
183 ↗ | (On Diff #44002) | Replace apostrophe |
252–253 ↗ | (On Diff #44002) | Is the newline between these lines necessary? |